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

fix(grid3d): Color fighting with clamp color is fixed. #1998

Merged
merged 19 commits into from
Apr 4, 2024
Merged

fix(grid3d): Color fighting with clamp color is fixed. #1998

merged 19 commits into from
Apr 4, 2024

Conversation

LeonidPolukhin
Copy link
Collaborator

Tiny tolerance value is now used to define when clamp color is to be used in the fragment shader.

@@ -26,7 +26,7 @@ vec4 getContinuousPropertyColor (float propertyValue) {

vec4 color = vec4(1.0, 1.0, 1.0, 1.0);
float x = (propertyValue - colorMapRangeMin) / (colorMapRangeMax - colorMapRangeMin);
if (x < 0.0 || x > 1.0) {
if (x < 0.0 - 1e-6 || x > 1.0 + 1e-6) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a story where we can see this impact ?

Copy link
Collaborator Author

@LeonidPolukhin LeonidPolukhin Apr 2, 2024

Choose a reason for hiding this comment

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

There was a temporary story for testing the impact but I don't realize what a permanent story is supposed to demonstrate.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Stories are tested automatically. Thus the storybook also acts as tests for webviz.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree with @t0oF-azpn, it would be good with a story that demonstrates the issue that this fix solves.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a story which can reproduce the issue if tolerance is not used in the fragment shader.

Copy link
Collaborator

@hkfb hkfb left a comment

Choose a reason for hiding this comment

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

Not sure I get it. Is this a way to trigger an update in the story?

Also, the functions look nearly identical. Could be refactored?

@LeonidPolukhin
Copy link
Collaborator Author

LeonidPolukhin commented Apr 4, 2024

Not sure I get it. Is this a way to trigger an update in the story?

Also, the functions look nearly identical. Could be refactored?

The functions are refactored.

The story shows the correct image for the case when there are cells with property values exactly equal to colorMapRange min or max. Without the fix in the fragment shader this image is corrupted with visual artefacts.

I have no idea how to trigger the artefacts in the storybook except exposing a special layer prop. But... do we really want a prop of the sort in the public API?

@LeonidPolukhin LeonidPolukhin merged commit 4b83d1a into equinor:master Apr 4, 2024
9 checks passed
hkfb pushed a commit that referenced this pull request Apr 4, 2024
## [0.22.1](https://github.com/equinor/webviz-subsurface-components/compare/[email protected]@0.22.1) (2024-04-04)

### Bug Fixes

* **grid3d:** Color fighting with clamp color is fixed. ([#1998](#1998)) ([4b83d1a](4b83d1a))
@hkfb
Copy link
Collaborator

hkfb commented Apr 4, 2024

🎉 This issue has been resolved in version [email protected] 🎉

The release is available on GitHub release

@hkfb hkfb added the released label Apr 4, 2024
@LeonidPolukhin LeonidPolukhin deleted the FixClampColoring branch June 18, 2024 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[NGRM] - Grid3DLayer: Incorrect coloring of property values equal to colorRange.
3 participants