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

HDR colors polylines with PolylineColorAppearance and PolylineMaterialAppearance differently #7920

Closed
hpinkos opened this issue Jun 7, 2019 · 7 comments · Fixed by #7924
Closed

Comments

@hpinkos
Copy link
Contributor

hpinkos commented Jun 7, 2019

Sandcastle

highDynamicRange = true:
image

highDynamicRange = false:
image

I kind of feel like HDR shouldn't be hitting primitive types like this where the user is defining a color. If I tell a polyline to be white, I want it to be white, not gray.

@mramato
Copy link
Contributor

mramato commented Jun 7, 2019

You labeled them both as HDR=false.

@hpinkos
Copy link
Contributor Author

hpinkos commented Jun 7, 2019

You labeled them both as HDR=false.

Shhhhhhhh.....

I updated the initial comment. The first one is with hdr enabled

@emackey
Copy link
Contributor

emackey commented Jun 7, 2019

Not sure I understand. Did we want them to be the same? I think HDR enables tone-mapping of colors almost across the board.

That said, the current look of PBR-enabled models is pretty bad these days. The glTF sample "Water Bottle" model looks plastic instead of aluminum. It wasn't that way a couple releases ago, not sure when it changed (I could bisect it, but I'm sure it would be some recent intentional HDR upgrade). I think the problem there is that people are fine-tuning the lighting/color balance to be optimal for a bunch of solid-white 3D Tiles buildings, and not testing with full PBR models. The two are almost mutually exclusive, they probably should have different shaders or render paths.

Anyway I'm way off topic here. Do we expect the lines to be exactly the same color, with and without HDR? I think they would need to be always excluded from tone-mapping in such a case.

@hpinkos
Copy link
Contributor Author

hpinkos commented Jun 7, 2019

From the perspective of someone with little to no idea of how HDR actually works, I think it's weird that the line I'm setting to Color.ORANGE doesn't look orange when HDR is enabled. I know it has also confused users with billboards wondering why they're a completely different color on the globe than in their .jpg files. But that's a slightly larger scoped problem than I meant to bring up in this issue.

The main problem here is that I set Color.ORANGE to all the polylines in the picture, but when HDR is enabled the bottom two polylines are a completely different color than the other polylines. I don't understand why the arrow material and the dashed line aren't the same color as the regular polyline color material.

@emackey
Copy link
Contributor

emackey commented Jun 7, 2019

Oh I see, with HDR enabled the ones on the ground are not the same as the ones at altitude. I was comparing HDR with non-HDR and I didn't spot that before.

That does seem like a bug. I wonder if tone-mapping is only applied to one set and not the other? That particular shade of orange is susceptible to having its hue changed by both tone-mapping and gamma correction, because only one of the three color channels is in a middle range, the others are zero and one.

@hpinkos
Copy link
Contributor Author

hpinkos commented Jun 7, 2019

The ones on the ground are using a PolylineColorAppearance and the other ones are using a PolylineMaterialAppearance. My guess is it's something related to that.

@hpinkos hpinkos changed the title HDR colors ground polylines differently HDR colors polylines with PolylineColorAppearance and PolylineMaterialAppearance differently Jun 7, 2019
@lilleyse
Copy link
Contributor

lilleyse commented Jun 8, 2019

This was a case of missing czm_gammaCorrect in those polyline shaders. Fixed in #7924.

As for HDR, IBL, PBR for models, part of the problem is the procedural lighting is just too bright: #7803.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants