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

Multiple globe materials makes the globe darker #8077

Merged
merged 10 commits into from
Aug 22, 2019

Conversation

ProjectBarks
Copy link
Contributor

@ProjectBarks ProjectBarks commented Aug 15, 2019

Fixes

  • Found bug where gamma was being correct on alpha, which had no impact
  • Fix that only adjusts gamma once

Notes:

While the gamma is adjusted per each material type I would like to make it so that it is adjusted on the final RGB values.

Screenshots:

Before

image image

After

image image

After with HDR

image image

In reference to: #7726

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Found bug where gamma was being correct on alpha, which had no impact

maded fix that only adjusts gamma once but looking to adjust gamma at the end
@cesium-concierge
Copy link

Thanks for the pull request @ProjectBarks!

  • ✔️ 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.

@lilleyse
Copy link
Contributor

How does this look with HDR enabled? It's important to still have a gamma correct somewhere if HDR is on.

@ProjectBarks
Copy link
Contributor Author

ProjectBarks commented Aug 15, 2019

@lilleyse updated with HDR screenshots

From what I saw, each material applies its own gamma correction so the one I removed was the duplicate in the fusion process. Let me know if that was incorrect.

Here is a sandcastle you can use to test.

@mramato
Copy link
Contributor

mramato commented Aug 19, 2019

@lilleyse does anything else need to happen here?

@lilleyse
Copy link
Contributor

I'll try to check this out later today.

@lilleyse
Copy link
Contributor

From what I saw, each material applies its own gamma correction so the one I removed was the duplicate in the fusion process. Let me know if that was incorrect.

This might not be true for all materials. Check out this Sandcastle example local vs. master where local doesn't apply gamma correction to the texture and it looks washed out.

Local
UNADJUSTEDNONRAW_thumb_a
Master
UNADJUSTEDNONRAW_thumb_b

@ProjectBarks
Copy link
Contributor Author

ProjectBarks commented Aug 20, 2019

I think I came to a good fix. Let me know if you find any other issues.

Copy link
Contributor

@lilleyse lilleyse left a comment

Choose a reason for hiding this comment

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

I'm not sure that this approach will handle every case, like if the composite material's diffuse is a totally separate value that doesn't reference the inner materials, but it handles the common cases well would be fine to merge. Thanks @ProjectBarks!

@ProjectBarks
Copy link
Contributor Author

@lilleyse I think it should now. Now we check if the component fuses multiple sub materials

@lilleyse
Copy link
Contributor

The updates look good, thanks @ProjectBarks!

@lilleyse lilleyse merged commit ef6c87c into CesiumGS:master Aug 22, 2019
@ProjectBarks ProjectBarks deleted the material-gamma-fix branch August 22, 2019 15:19
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