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 broken UGRID/large map and PRegionSelector edge case #3584

Closed
wants to merge 8 commits into from

Conversation

sgpearse
Copy link
Collaborator

Fixes #3575 by removing code in the TwoDRenderer that was redundantly applying another set of transforms to the data.

Fixes #3583 by allowing the PRegionSelector to optionally receive a ControlExecutive instance to operate with the extents of the entire data domain. This was needed in the ImageRenderer where there is not always a 2D variable to get grid extents from.

@sgpearse sgpearse requested a review from StasJ April 17, 2024 19:50
@sgpearse
Copy link
Collaborator Author

sgpearse commented Apr 17, 2024

Hey @StasJ, do we have a written procedure to run the Dataset Vis Regression tests? I'd like to run this code through them for validation.

@StasJ
Copy link
Collaborator

StasJ commented Apr 17, 2024

Hey @StasJ, do we have a written procedure to run the Dataset Vis Regression tests? I'd like to run this code through them for validation.

Yes, the scripts have documentation in them and it should work out of the box on casper. You may need to update dataRootDir in the config file.

@sgpearse
Copy link
Collaborator Author

Hey @StasJ, do we have a written procedure to run the Dataset Vis Regression tests? I'd like to run this code through them for validation.

Yes, the scripts have documentation in them and it should work out of the box on casper. You may need to update dataRootDir in the config file.

Thanks. Could you share the name of the script and the directory where it resides?

@StasJ
Copy link
Collaborator

StasJ commented Apr 17, 2024

Hey @StasJ, do we have a written procedure to run the Dataset Vis Regression tests? I'd like to run this code through them for validation.

Yes, the scripts have documentation in them and it should work out of the box on casper. You may need to update dataRootDir in the config file.

Thanks. Could you share the name of the script and the directory where it resides?

Sure, it is in test_apps/render_regression_tests

@sgpearse sgpearse marked this pull request as draft April 19, 2024 14:20
@sgpearse sgpearse marked this pull request as ready for review April 23, 2024 17:49
Copy link
Collaborator

@StasJ StasJ left a comment

Choose a reason for hiding this comment

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

For #3583, it is preferable not to give GUI elements access to the control exec when they don't need them. The issue within the region selector can be instead fixed on the backend by ensuring it returns a valid variable.

Either way, this does not fix the underlying issue which is that the image renderer will crash for any zero width region.

For #3575, you said the fix works by "removing code in the TwoDRenderer that was redundantly applying another set of transforms" but the code was just moved up, not removed, so the redundancy would still be occurring?

@sgpearse
Copy link
Collaborator Author

For #3583, it is preferable not to give GUI elements access to the control exec when they don't need them. The issue within the region selector can be instead fixed on the backend by ensuring it returns a valid variable.

Either way, this does not fix the underlying issue which is that the image renderer will crash for any zero width region.

In this dataset, there are no 2D variables to query a region from so we need to resort to the domain extents, which is why the CE is needed here.

For #3575, you said the fix works by "removing code in the TwoDRenderer that was redundantly applying another set of transforms" but the code was just moved up, not removed, so the redundancy would still be occurring?

That's correct. I thought it was redundant in my original assessment but it turns out it is still needed.

@StasJ
Copy link
Collaborator

StasJ commented Apr 29, 2024

In this dataset, there are no 2D variables to query a region from so we need to resort to the domain extents, which is why the CE is needed here.

This information could be acquired from the params without needing the CE.

That's correct. I thought it was redundant in my original assessment but it turns out it is still needed.

Could the TwoDDataRenderer specific code be placed inside the class rather than having if (is TwoDDataRenderer) in the universal render code?

@sgpearse
Copy link
Collaborator Author

This information could be acquired from the params without needing the CE.

Hmm I do not see a way to do this. We need to do two things:

  1. Set the slider range
  2. Set the slider value

We can acquire a box to set the slider values, but we have no way of getting the range without either having an active variable or the CE. Am I missing something here?

Could the TwoDDataRenderer specific code be placed inside the class rather than having if (is TwoDDataRenderer) in the universal render code?

Not that I see, but I wish there was. The problem is that we need to apply a height offset before Renderer::ApplyTransform(). If we apply the offset after transforming (like what we were doing previously) then the height gets multiplied by the Z scale, sending the map off into outer space. In #3575, the map was being rendered, but just a million miles behind the camera.

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.

ImageRenderer cannot handle datasets without 2D vars UGRID/large image renderer empty
3 participants