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.

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

Refactoring mvr.py #203

Merged
merged 24 commits into from
Jul 23, 2024
Merged

Refactoring mvr.py #203

merged 24 commits into from
Jul 23, 2024

Conversation

nrgsille76
Copy link
Contributor

Complete refactor of mvr.py including:

  1. Avoid importing meshes multiple times
  2. Use collection instances for symbol objects
  3. Use already imported collections instead of creating new
  4. Use object uuids instead of hashlib or collection names
  5. Create custom properties for MVR class, name, uuid and reference
  6. Fixed matrix transformations
  7. Fixed names if they are doubles
  8. Importing is now about 10 times faster

@vanous
Copy link
Collaborator

vanous commented Jul 22, 2024

I have also tested the speed, with the capture demo file.

The current mvr import takes:

INFO MVR scene loaded in 314.9368 sec.

And loads 78 fixtures.

The refactor takes:

INFO MVR scene loaded in 372.4841 sec.

But actually loads only 28 fixtures. So if all fixtures were loaded, it will be even a bit slower (This is fine).

Testing file attached: Capture_MVR.zip

@nrgsille76
Copy link
Contributor Author

nrgsille76 commented Jul 22, 2024

@vanous Fixed with 04ef1e1

It took 421.8280 sec in my test, now all fixtures seem to be present.

Screenshot demo file

@vanous
Copy link
Collaborator

vanous commented Jul 22, 2024

I have been testing further: first import works (with the changes i mentioned in the comments). Then, if you re-import the same file again, what happens is that some geometries get duplicated, which should not happen. For that, i use the scene_objects.mvr from the pymvr repo, it loads fast and has good features for testing.

@nrgsille76
Copy link
Contributor Author

nrgsille76 commented Jul 22, 2024

I have been testing further: first import works (with the changes i mentioned in the comments). Then, if you re-import the same file again, what happens is that some geometries get duplicated, which should not happen. For that, i use the scene_objects.mvr from the pymvr repo, it loads fast and has good features for testing.

I see, this indeed should not happen and I catched that issue in my test setup, I will try to fix this.

@nrgsille76
Copy link
Contributor Author

nrgsille76 commented Jul 22, 2024

@vanous Should now be fixed with commit ebcaf9a

Edit: catched some exceptions if empty layers are already removed dcef0eb

@nrgsille76
Copy link
Contributor Author

nrgsille76 commented Jul 22, 2024

@vanous I tried a plenty of methods to check for existing objects. It is no problem for all regular geometries but for symbols it is not easy because it can be the same object with a different transformation, I have no idea how to identify the object if it has the same filename and symdef but is linked to another scene object because the aux data has to be loaded first to have them available if symbols got imported
Maybe you got some ideas for that, It would be no problem if a geometry object had its own uuid

@vanous
Copy link
Collaborator

vanous commented Jul 22, 2024

Thank you. I would have to have a look how it was done before and dive deeper into the mvr code. I remember that for the Geometry3D i generate a hash to be unique based on name and transform here, so could we perhaps do something similar in this case?

@nrgsille76
Copy link
Contributor Author

nrgsille76 commented Jul 22, 2024

@vanous
Finally made it, looks a bit wierd but works on my side. Latest commit: f7b438a

Edit: Found a simpler solution: f04331c

@vanous
Copy link
Collaborator

vanous commented Jul 23, 2024

@nrgsille76 thank you for all the work! Ready to merge?

existing_fixture.build(
unique_name,
f"{fixture.name} {layer_index}-{fixture_index}",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here we need to use the unique name, otherwise subsequent import of the same file will fail. This is because in BlenderDMX, the collection name is the fixture name, which is not ideal. In the future, fixture name will be independent of collection name.

dmx.addFixture(
unique_name,
f"{fixture.name} {layer_index}-{fixture_index}",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here - we need to use the unique name, otherwise subsequent import of the same file will fail. This is because in BlenderDMX, the collection name is the fixture name, which is not ideal. In the future, fixture name will be independent of collection name.

if len(layer_collection.all_objects) == 0:
bpy.context.scene.collection.children.unlink(layer_collection)

load_mvr(self, file_name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nice to add the import configuration here in the future, this will make also testing much easier - import only fixtures, or import only some objects.

folder_path, extracted, layer_collection, fixture_group)

if len(layer_collection.all_objects) == 0:
layer_collect.children.unlink(layer_collection)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to handle if the collection exists:, this works:

        if len(layer_collection.all_objects) == 0:
            if layer_collection.name in layer_collect.children:
                layer_collect.children.unlink(layer_collection)

@vanous vanous merged commit d10abd7 into open-stage:main Jul 23, 2024
1 check passed
@vanous
Copy link
Collaborator

vanous commented Jul 23, 2024

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants