Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

3D export fixes and tests #4368

Merged
merged 5 commits into from
May 9, 2024
Merged

3D export fixes and tests #4368

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0266240
WIP 3D export fixes and tests
swheaton May 7, 2024
9c66284
print
swheaton May 7, 2024
06162b5
errant copypasta
swheaton May 8, 2024
41d07c7
move rewrite asset logic to scene_3d
swheaton May 8, 2024
1a8b9ba
cleanup 3d import/export tests a bit
swheaton May 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions fiftyone/core/threed/scene_3d.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,13 +248,57 @@ def traverse(self, include_self=False):

Args:
include_self: whether to include the current node in the traversal

Yields:
:class:`Object3D`

"""
if include_self:
yield self

for child in self.children:
yield from child.traverse(include_self=True)

def update_asset_paths(self, asset_rewrite_paths: dict):
"""Update asset paths in this scene according to an input dict mapping.

Asset path is unchanged if it does not exist in ``asset_rewrite_paths``

Args:
asset_rewrite_paths: ``dict`` mapping asset path to new asset path

Returns:
``True`` if the scene was modified.
"""
scene_modified = False
for node in self.traverse():
for path_attribute in node._asset_path_fields:
asset_path = getattr(node, path_attribute, None)
new_asset_path = asset_rewrite_paths.get(asset_path)

if asset_path is not None and asset_path != new_asset_path:
setattr(node, path_attribute, new_asset_path)
scene_modified = True

# modify scene background paths, if any
if self.background is not None:
if self.background.image is not None:
new_asset_path = asset_rewrite_paths.get(self.background.image)
if new_asset_path != self.background.image:
self.background.image = new_asset_path
scene_modified = True

if self.background.cube is not None:
new_cube = [
asset_rewrite_paths.get(face)
for face in self.background.cube
]
if new_cube != self.background.cube:
self.background.cube = new_cube
scene_modified = True

return scene_modified
Comment on lines +262 to +300
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider enhancing traceability and robustness in asset path updates.

  • It would be beneficial to add logging at key points in this method to trace the changes being made to asset paths, which can be crucial for debugging and verifying the behavior in production.
  • Additionally, consider handling the scenario where new_asset_path is None. Currently, if asset_rewrite_paths.get(asset_path) returns None, it will replace a valid path with None, which might not be the intended behavior. You might want to add a condition to check if new_asset_path is not None before setting it.
+                if asset_path is not None and new_asset_path is not None and asset_path != new_asset_path:
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
def update_asset_paths(self, asset_rewrite_paths: dict):
"""Update asset paths in this scene according to an input dict mapping.
Asset path is unchanged if it does not exist in ``asset_rewrite_paths``
Args:
asset_rewrite_paths: ``dict`` mapping asset path to new asset path
Returns:
``True`` if the scene was modified.
"""
scene_modified = False
for node in self.traverse():
for path_attribute in node._asset_path_fields:
asset_path = getattr(node, path_attribute, None)
new_asset_path = asset_rewrite_paths.get(asset_path)
if asset_path is not None and asset_path != new_asset_path:
setattr(node, path_attribute, new_asset_path)
scene_modified = True
# modify scene background paths, if any
if self.background is not None:
if self.background.image is not None:
new_asset_path = asset_rewrite_paths.get(self.background.image)
if new_asset_path != self.background.image:
self.background.image = new_asset_path
scene_modified = True
if self.background.cube is not None:
new_cube = [
asset_rewrite_paths.get(face)
for face in self.background.cube
]
if new_cube != self.background.cube:
self.background.cube = new_cube
scene_modified = True
return scene_modified
def update_asset_paths(self, asset_rewrite_paths: dict):
"""Update asset paths in this scene according to an input dict mapping.
Asset path is unchanged if it does not exist in ``asset_rewrite_paths``
Args:
asset_rewrite_paths: ``dict`` mapping asset path to new asset path
Returns:
``True`` if the scene was modified.
"""
scene_modified = False
for node in self.traverse():
for path_attribute in node._asset_path_fields:
asset_path = getattr(node, path_attribute, None)
new_asset_path = asset_rewrite_paths.get(asset_path)
if asset_path is not None and new_asset_path is not None and asset_path != new_asset_path:
setattr(node, path_attribute, new_asset_path)
scene_modified = True
# modify scene background paths, if any
if self.background is not None:
if self.background.image is not None:
new_asset_path = asset_rewrite_paths.get(self.background.image)
if new_asset_path != self.background.image:
self.background.image = new_asset_path
scene_modified = True
if self.background.cube is not None:
new_cube = [
asset_rewrite_paths.get(face)
for face in self.background.cube
]
if new_cube != self.background.cube:
self.background.cube = new_cube
scene_modified = True
return scene_modified


def get_scene_summary(self):
"""Returns a summary of the scene."""
node_types = Counter(map(type, self.traverse()))
Expand Down
46 changes: 8 additions & 38 deletions fiftyone/utils/data/exporters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1171,6 +1171,7 @@ def _handle_fo3d_file(self, fo3d_path, fo3d_output_path, export_mode):
scene = fo3d.Scene.from_fo3d(fo3d_path)
asset_paths = scene.get_asset_paths()

input_to_output_paths = {}
for asset_path in asset_paths:
if not os.path.isabs(asset_path):
absolute_asset_path = os.path.join(
Expand All @@ -1181,12 +1182,15 @@ def _handle_fo3d_file(self, fo3d_path, fo3d_output_path, export_mode):

seen = self._filename_maker.seen_input_path(absolute_asset_path)

if seen:
continue

asset_output_path = self._filename_maker.get_output_path(
absolute_asset_path
)
input_to_output_paths[asset_path] = os.path.relpath(
asset_output_path, os.path.dirname(fo3d_output_path)
)

if seen:
continue

if export_mode is True:
etau.copy_file(absolute_asset_path, asset_output_path)
Expand All @@ -1195,41 +1199,7 @@ def _handle_fo3d_file(self, fo3d_path, fo3d_output_path, export_mode):
elif export_mode == "symlink":
etau.symlink_file(absolute_asset_path, asset_output_path)

is_scene_modified = False

for node in scene.traverse():
path_attribute = next(
(
attr
for attr in fo3d.fo3d_path_attributes
if hasattr(node, attr)
),
None,
)

if path_attribute is not None:
asset_path = getattr(node, path_attribute)

is_nested_path = os.path.split(asset_path)[0] != ""

if asset_path is not None and is_nested_path:
setattr(node, path_attribute, os.path.basename(asset_path))
is_scene_modified = True

# modify scene background paths, if any
if scene.background is not None:
if scene.background.image is not None:
scene.background.image = os.path.basename(
scene.background.image
)
is_scene_modified = True

if scene.background.cube is not None:
scene.background.cube = [
os.path.basename(face_path)
for face_path in scene.background.cube
]
is_scene_modified = True
is_scene_modified = scene.update_asset_paths(input_to_output_paths)

if is_scene_modified:
# note: we can't have different behavior for "symlink" because
Expand Down
Loading
Loading