-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Solve struct ellipsoid caused the model to dark #7944
Conversation
Solved the problem that struct ellipsoid caused the model to darken on Qualcomm platform
Thanks for the pull request @verybigzhouhai!
Reviewers, don't forget to make sure that:
|
Fixed #7651 |
@OmarShehata Hi,Now I am modifying the relevant code of the ellipsoid struct. Do you think it is acceptable? |
This is ready! |
Just to close the loop, this is the same fix as implemented in #7911 right? |
@OmarShehata yes, it's exactly the same. |
@OmarShehata Will you review this? |
I can confirm this fixes the issue at #6622. I believe this also fixes #7651 and #7403. Testing on the physically based Sandcastle, in the hosted Sandcastle on Android: In this branch: @verybigzhouhai my main notes here are:
|
@OmarShehata I am very happy that the function is normal in your testing. |
@verybigzhouhai I'm curious about the unit tests that fail using that. Can you list a few of them and the errors you get? |
remove function czm_getWgs84EllipsoidEC remove struct czm_ellipsoid add Constant czm_ellipsoid_radii add Constant czm_ellipsoid_inverseRadii
Delete unused files
3dtile dark02
Delete ellipsoidNew.glsl
modify constant name
@OmarShehata I found some errors of my own, which will not lead to unit test failure now. Now I add two constants and delete files that do not need to be used. Updates have been submitted,you can check whether they meet the requirements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great @verybigzhouhai ! Just a minor doc comment and I think this will be ready to merge.
@verybigzhouhai one more thing: Can you update
|
add note and update CHANGES.md
Thank you very much for your guidance, @OmarShehata now it should be ready to be merged. |
Great - I'm confident merging this since it's largely just rewriting structs to use builtin constants. It works in Chrome, Firefox, Edge, and IE11. It works correctly in Chrome on Android now. @lilleyse if you want to skim the changes later when you get a chance in case I missed any shader styling thing I wasn't aware of, that would be helpful! |
If this is a first-time contribution, we'd like to share it. @verybigzhouhai do you have a Twitter handle? |
@slchow twitter handle @bigzhouhai https://mobile.twitter.com/bigzhouhai |
Solved the problem that struct ellipsoid caused the model to darken on Qualcomm platform