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

Solve struct ellipsoid caused the model to dark #7944

Merged
merged 14 commits into from
Jun 28, 2019
Merged

Solve struct ellipsoid caused the model to dark #7944

merged 14 commits into from
Jun 28, 2019

Conversation

verybigzhouhai
Copy link
Contributor

Solved the problem that struct ellipsoid caused the model to darken on Qualcomm platform

Solved the problem that struct ellipsoid caused the model to darken on Qualcomm platform
@cesium-concierge
Copy link

Thanks for the pull request @verybigzhouhai!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ 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.
  • Works (or fails gracefully) in IE11.

@verybigzhouhai
Copy link
Contributor Author

Fixed #7651

@verybigzhouhai
Copy link
Contributor Author

@OmarShehata Hi,Now I am modifying the relevant code of the ellipsoid struct. Do you think it is acceptable?

@verybigzhouhai
Copy link
Contributor Author

This is ready!

@OmarShehata
Copy link
Contributor

Just to close the loop, this is the same fix as implemented in #7911 right?

@verybigzhouhai
Copy link
Contributor Author

@OmarShehata yes, it's exactly the same.

@verybigzhouhai
Copy link
Contributor Author

@OmarShehata Will you review this?

@OmarShehata
Copy link
Contributor

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:

65103522_484235775660795_8115887609747603456_n

In this branch:

65482229_472193313558138_6022643596195266560_n

@verybigzhouhai my main notes here are:

@verybigzhouhai
Copy link
Contributor Author

@OmarShehata I am very happy that the function is normal in your testing.
I did use some constants to replace the structure at the beginning, but it caused a lot of unit tests to fail. I don't know how to avoid or need to modify a lot of unit test code, so I used the current modification method.
Even if we use repeatable constants, I don't think this is the best way to modify them, because the structure itself is hoping to make the program simpler, so I think the best way to do this is to dynamically before compiling the shader. Replace the structure, but this approach may not be that simple, I have raised a similar issue here, https://stackoverflow.com/questions/56479172/about-remove-replace-struct , if we can do this, could make the code Keep the original elegance.
What do you think about the above, if nothing, I will modify it again according to what you mentioned.

@OmarShehata
Copy link
Contributor

@verybigzhouhai I'm curious about the unit tests that fail using that. Can you list a few of them and the errors you get?

Zhouhai and others added 8 commits June 26, 2019 10:47
remove function czm_getWgs84EllipsoidEC
remove struct czm_ellipsoid

add Constant czm_ellipsoid_radii
add Constant czm_ellipsoid_inverseRadii
Delete unused files
This reverts commit f383685, reversing
changes made to b2588e4.
modify constant name
@verybigzhouhai
Copy link
Contributor Author

@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.

Copy link
Contributor

@OmarShehata OmarShehata left a 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.

@OmarShehata
Copy link
Contributor

@verybigzhouhai one more thing: Can you update CHANGES.md to add a bullet point under "fixes"? It should say something like:

  • Fixed a bug that caused 3D models to appear darker on Android devices. #7944

Zhouhai and others added 2 commits June 28, 2019 09:21
@verybigzhouhai
Copy link
Contributor Author

Thank you very much for your guidance, @OmarShehata now it should be ready to be merged.

@OmarShehata
Copy link
Contributor

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!

@slchow
Copy link
Contributor

slchow commented Jun 28, 2019

If this is a first-time contribution, we'd like to share it. @verybigzhouhai do you have a Twitter handle?

@verybigzhouhai
Copy link
Contributor Author

verybigzhouhai commented Jun 28, 2019

@slchow twitter handle @bigzhouhai https://mobile.twitter.com/bigzhouhai

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