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

Use DataMgrUtils instead of CE #3608

Merged
merged 2 commits into from
May 14, 2024
Merged

Use DataMgrUtils instead of CE #3608

merged 2 commits into from
May 14, 2024

Conversation

sgpearse
Copy link
Collaborator

@sgpearse sgpearse commented May 1, 2024

@StasJ, I looked into using RenderParams to query a random 3D variable but there does not seem to be a way to do this. The RenderParams gets instantiated with a dimensionality, and has no evident way to query variables outside of their given dimension. The only path forward I found was to use DataMgrUtils, shown here.

Also, the CE method used in PR #3584 does in fact find the domain extents by taking the union of all variables in the dataset. Not taking the union of the extents could break the PRegionSelector if we don't land on a variable that encloses the entire domain, which could happen. WRF can contain soil and fire grids that lie outside or within the meteorological grids that we typically deal with.

Feel free to pick whichever solution you like.

Fixes #3575 and #3583.

@sgpearse sgpearse requested a review from StasJ May 1, 2024 18:03
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.

Thanks, this solution requires much fewer changes!

Like I mentioned in the original PR review, however, this does not fix the underlying issue causing the crash in #3583, which is that the image renderer will crash for any zero width region.

@sgpearse
Copy link
Collaborator Author

sgpearse commented May 8, 2024

@StasJ I chose to return from functions that were VAsserting when the texture size was too small. I don't think it's necessary to report an error to the user when the shrink the region to something that won't render anything.

@sgpearse sgpearse requested a review from StasJ May 8, 2024 19:17
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.

Looks good, thanks!

@NihanthCW NihanthCW merged commit 5800885 into main May 14, 2024
1 check passed
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.

UGRID/large image renderer empty
3 participants