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

generate unique names for nodes in 3d models #795

Conversation

eeropomell
Copy link
Contributor

This PR introduces a dictionary in Model.cs for having a unique id for each node in a 3d model. It fixes string handling related issues in ModelWidget, #794

Todo:

  • Measure Performance: when the dictionary is initialized, the entire transform hierarchy is traversed during model load.
  • Currently it rewrites each node's name with the uid. This should probably be changed so the transform has the real name and instead of using Transform.Find() for node search, a custom method is implemented.
    • The user doesn't see the node names but it still makes debugging harder.

@eeropomell eeropomell changed the base branch from main to feature/icosa-integration December 7, 2024 00:54
@eeropomell eeropomell marked this pull request as ready for review December 7, 2024 11:10
@mikeage mikeage added the bugfix Something has been fixed label Dec 7, 2024
@mikeage
Copy link
Member

mikeage commented Dec 7, 2024

Please make sure you're either using pre-commit locally or something else (Rider?) that handles the formatting as defined in .editorconfig

This commit improves developer experience as the original node name is kept visible, and the UID is appended to the end

[CI BUILD DEV] [CI BUILD]
@eeropomell eeropomell changed the title Fix/modelwidget subtree string handling generate unique names for nodes in 3d models Dec 8, 2024
@eeropomell
Copy link
Contributor Author

This PR is ready for review @andybak

TLDR:

  • performance-wise, the implementation is good enough. On Meta Quest 3 it takes < 3ms to traverse a tree of 1000 nodes. Most users are importing models with < 1000 nodes. If this is to be optimized further, we should before that figure out if there are users who are using models that have > 100k or > 1 million nodes.

  • the current implementation appends the node's UID to the node's name (e.g., "root uid: 1234567"), which is a simple and functional solution. We don't have to modify code in e.g ModelWidget.cs. A future improvement could focus on a cleaner implementation of unique identifiers without modifying the node names.

Profiling:

I didn't want there to be an insane frame freeze when the user imports a 3d model and the node tree is traversed and unique names are generated.

Some notes:

  1. The profiling was done on Meta Quest 3, which has a Qualcomm Snapdragon XR2 CPU.
  2. The performance of node tree traversal and unique name generation depends on CPU and RAM, not e.g., GPU. My current understanding is that other VR headsets OB is supported on have about equally powerful CPUs. For example, Vive Elite XR also has a Qualcomm Snapdragon XR2 CPU.
  3. When profiling, the main thing that matters in the input is how many nodes the tree has. For example, how much geometry data the model has doesn't at least directly affect how fast we can traverse the node tree.

For a model with 1000 empty nodes, it takes ~3ms to traverse the tree and generate unique names:
Here is an image from Unity Profiler, showing the EndCreatePrefab marker taking 2.9ms:
95189544d37eb4e768d52f197a4e2122111

This video shows importing models with 100, 1k, 10k, and 100k nodes. The model has completed loading and the unique names have been generated when the preview appears (which for these models is empty as there's no geometry data). Note that the node tree traversal is only a part of the import.

  • With 100 and 1k, import is instant
  • With 10k, import is about 0.5 seconds.
  • With 100k nodes, the entire import takes a few seconds, and there's a ~1 sec frame freeze at the end, which is caused by the node tree traversal.
0376-0889.mp4

@andybak andybak merged commit c228c1a into icosa-foundation:feature/icosa-integration Dec 13, 2024
41 of 42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Something has been fixed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants