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

Adds dynamic model/node transformations to ModelExperimentalNode #9989

Merged
merged 15 commits into from
Jan 7, 2022

Conversation

sanjeetsuhag
Copy link
Contributor

@sanjeetsuhag sanjeetsuhag commented Jan 3, 2022

This PR adds support for dynamically changing node transforms in ModelExperimental. It also adds the ability to traverse the scene graph of ModelExperimentaNode objects.

Architecture

model.modelMatrix update

Update Stage - Frame 1 (2)

node.transform update

Update Stage - Frame 2

To-Do

  • Add a way to update the draw commands with the new modelMatrix
  • Fix child transform bug
  • Update documentation
  • Add tests

Testing

Truck Sandcastle
transform

@cesium-concierge
Copy link

Thanks for the pull request @sanjeetsuhag!

  • ✔️ Signed CLA found.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.

Source/Scene/ModelExperimental/ModelExperimentalNode.js Outdated Show resolved Hide resolved
Source/Scene/ModelExperimental/ModelExperimentalNode.js Outdated Show resolved Hide resolved
this.sceneGraph = options.sceneGraph;

/**
* The indices of the children of this node in the runtime nodes array of the scene graph.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will users expect the exact same indices as in the glTF? Because the runtimeNodes array is generated from a post-order depth-first traversal of the scene graph, a node's glTF ID will not necessarily be a match to its index in the runtimeNodes array.

Base automatically changed from model-components-transform to main January 4, 2022 01:07
@sanjeetsuhag sanjeetsuhag changed the title Adds transform and getChild to ModelExperimentalNode Adds dynamic model/node transformations to ModelExperimentalNode Jan 6, 2022
@sanjeetsuhag sanjeetsuhag marked this pull request as ready for review January 7, 2022 05:54
@sanjeetsuhag sanjeetsuhag requested a review from ptrgags January 7, 2022 13:47
@sanjeetsuhag
Copy link
Contributor Author

@ptrgags This is ready for review.

Copy link
Contributor

@ptrgags ptrgags left a comment

Choose a reason for hiding this comment

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

@sanjeetsuhag just a few minor comments

Source/Scene/ModelExperimental/buildDrawCommands.js Outdated Show resolved Hide resolved
Source/Scene/ModelExperimental/ModelExperimentalNode.js Outdated Show resolved Hide resolved
Source/Scene/ModelExperimental/ModelExperimentalNode.js Outdated Show resolved Hide resolved
Source/Scene/ModelExperimental/buildDrawCommands.js Outdated Show resolved Hide resolved
@ptrgags
Copy link
Contributor

ptrgags commented Jan 7, 2022

Looks good to me, @lilleyse do you want to take a look before I merge?

@lilleyse
Copy link
Contributor

lilleyse commented Jan 7, 2022

@ptrgags feel free to merge

@ptrgags ptrgags merged commit b72d8fa into main Jan 7, 2022
@ptrgags ptrgags deleted the model-experimental-node-transforms branch January 7, 2022 20:29
@ptrgags
Copy link
Contributor

ptrgags commented Jan 7, 2022

Thanks @sanjeetsuhag!

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.

4 participants