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

Ensure spec parity between Model and ModelExperimental #10600

Merged
merged 10 commits into from
Jul 28, 2022

Conversation

j9liu
Copy link
Contributor

@j9liu j9liu commented Jul 26, 2022

This PR reorganizes ModelExperimentalSpec, and adds a handful of new specs to ModelExperimental and related classes. These are specs from ModelSpec that didn't have existing ModelExperimental counterparts.

There are two xit-ed unit tests. We need to return to these later and fix them:

  • "disables culling"
  • "renders model with emissive texture"

For comparison, here's how the latter test looks:

Model ModelExperimental
image image

@j9liu j9liu requested a review from ptrgags July 26, 2022 21:27
@cesium-concierge
Copy link

Thanks for the pull request @j9liu!

  • ✔️ Signed CLA found.

Reviewers, don't forget to make sure that:

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

@j9liu j9liu force-pushed the model-experimental-spec-parity branch from 3029209 to d1a4dcb Compare July 27, 2022 13:40
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.

@j9liu specs are running fine though when I compare the spec lists looks like there's quite a few tests the old Model had that aren't in ModelExperimental. Maybe that was intentional (e.g. some render tests might be better handled in other files) but wasn't sure.

@j9liu
Copy link
Contributor Author

j9liu commented Jul 27, 2022

Two things to note from this PR:

  • GltfJsonLoader automatically upgrades glTF 1.0 to glTF 2.0. We should remove that officially when we deprecate glTF 1.0 models.
  • Trying to include a check for the glTF magic in GltfLoader broke a lot of unit tests, so I'm not doing anything with that for now.

@j9liu
Copy link
Contributor Author

j9liu commented Jul 27, 2022

@ptrgags updated!

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.

@j9liu almost there, just a couple comments!

Specs/Scene/ModelExperimental/ModelExperimentalSpec.js Outdated Show resolved Hide resolved
Source/Scene/ModelExperimental/ModelExperimental.js Outdated Show resolved Hide resolved
@j9liu
Copy link
Contributor Author

j9liu commented Jul 28, 2022

@ptrgags updated!

@ptrgags
Copy link
Contributor

ptrgags commented Jul 28, 2022

Updates look good, thanks @j9liu!

@ptrgags ptrgags merged commit db04033 into replace-model Jul 28, 2022
@ptrgags ptrgags deleted the model-experimental-spec-parity branch July 28, 2022 14:45
@ptrgags ptrgags mentioned this pull request Aug 5, 2022
4 tasks
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