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

Geometry nodes support #160

Closed
wants to merge 80 commits into from
Closed

Geometry nodes support #160

wants to merge 80 commits into from

Conversation

seandillon92
Copy link
Contributor

Support for sending instance info from geometry nodes

Sean Dillon added 6 commits February 2, 2022 18:18
@unity-cla-assistant
Copy link

unity-cla-assistant commented Feb 8, 2022

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Sean Dillon seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Contributor Author

@seandillon92 seandillon92 left a comment

Choose a reason for hiding this comment

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

Hey @sindharta here is a self-review with some changes I would like to make in this PR. On a higher level, I noticed that while the VariantData works well for manual sync, auto sync requires some state management around what is dirty or not.

A quick and dirty solution to the latest commit is to change the Transform checksum to take into account the variant data checksums.

A cleaner solution would be to create a manager class for instances, similar to the entityManager. That would allow us to create our own serializable objects for instances and additional info if required. It would also decouple the instances info from meshes, so we should be able to send scene updates for instances without re-sending the mesh data.

Let me know what you think about the plan, any feedback would be appreciated!

Plugins~/Src/MeshSyncClientBlender/msblenContext.cpp Outdated Show resolved Hide resolved
Plugins~/Src/MeshSyncClientBlender/msblenGeometryNodes.h Outdated Show resolved Hide resolved
@sindharta
Copy link
Collaborator

sindharta commented Feb 18, 2022

A general question. Without this PR on the Meshsync side: 549, will Unity just simply ignore the instance info ?
Might be worth testing to ensure it doesn't crash.

@sindharta
Copy link
Collaborator

Another idea is, can we make a certain user interaction to set a flag for sending geometry node variant data, instead of having to calculate the checksum of all variant data.

@seandillon92
Copy link
Contributor Author

A general question. Without this PR on the Meshsync side: 549, will Unity just simply ignore the instance info ? Might be worth testing to ensure it doesn't crash.

I think it will be safe. Unity will simply not check for the variant data. I will definitely test it before merging in any case.

@seandillon92
Copy link
Contributor Author

Another idea is, can we make a certain user interaction to set a flag for sending geometry node variant data, instead of having to calculate the checksum of all variant data.

That's a good point. At the moment I have started moving away from the VariantData approach but if we were to merge the variant data solution, I will make sure to implement this

…odes class to a geometry nodes utility class
Copy link
Contributor Author

@seandillon92 seandillon92 left a comment

Choose a reason for hiding this comment

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

Implemented the Instance manager class and updated the scene exported to use it. Need to validate thread safety.

@seandillon92
Copy link
Contributor Author

Moved to #185 to fix issue with CLA client

@sindharta sindharta deleted the geometry_nodes_support branch July 29, 2022 04:06
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.

5 participants