GLVis / glvis

Lightweight OpenGL tool for accurate and flexible finite element visualization
http://glvis.org
BSD 3-Clause "New" or "Revised" License
249 stars 51 forks source link

Higher auto-refinement limits #285

Closed tzanio closed 1 month ago

tzanio commented 2 months ago

This is meant to address the issue where small meshes are curved but refined versions of them appear to lose the curvature in GLVis. See also this task in https://github.com/GLVis/glvis/pull/284

Command-line option to ensure minimal level of refinement to large meshes don't show as linear by default (ping: @dylan-copeland, @psocratis)?

The reasons for that behavior are the auto-refinement limits, which previously were:

Both of these variables are set in lib/vsdata.cpp

After considering several options, I suggest that we just increase those variables to 32 and 5M respectively.

These limits seems to work fine on modern hardware and the waiting time for large meshes is a few seconds on my Mac.

@psocratis and @dylan-copeland -- do you mind trying this on your use cases and reporting if it works well?

tzanio commented 1 month ago

I created a poll for this, please vote here: https://github.com/GLVis/glvis/discussions/292

najlkin commented 1 month ago

How about this? :wink: Please test it, it cannot pass the regression tests for obvious reasons and I do not have much time for it.

najlkin commented 1 month ago

Now all tests run, but there are some visible differences in the zoom level interestingly 🤔 .

justinlaughlin commented 1 month ago

Now all tests run, but there are some visible differences in the zoom level interestingly 🤔 .

just making sure - this isn't the difference between the test.*.saved.png vs test.fail.*.saved.png files right? Those filenames are misleading (in my opinion) because the test.fail.* files are actually the output of the negative test (making sure that the tests fail when they should). All of the "negative tests" are zoomed in so that they fail.

In order to compare to the baselines you have to download the baselines separately. I'm looking at the test outputs and see some differences but I don't see any visible differences in zoom.

e.g. (test.rehmos.saved.png) left = baseline, right = test output. I manually zoomed in so its not aligned perfectly. image

najlkin commented 1 month ago

Aha, ok, but the refinement levels are different. I am working on rebalancing that :wink:

justinlaughlin commented 1 month ago

I don't mean to derail this too much... but I was thinking maybe the baselines could be included as part of the artifact + the negative test outputs could be renamed into something less misleading (e.g. test.negative.*)? I think the "zoomed" failed test images are not self-explanatory without looking at the source-code, and I believe this will likely cause confusion for others in the future too haha. What do you think?

najlkin commented 1 month ago

I don't mean to derail this too much... but I was thinking maybe the baselines could be included as part of the artifact + the negative test outputs could be renamed into something less misleading (e.g. test.negative.*)? I think the "zoomed" failed test images are not self-explanatory without looking at the source-code, and I believe this will likely cause confusion for others in the future too haha. What do you think?

That would make a lot of sense to me 👍 I am still a bit confused how that works 🙃

najlkin commented 1 month ago

Heey, we are passing 🥳 I rebalanced the tests in GLVis/data#3 , which we should merge before/with this one.

tzanio commented 1 month ago

Remove "[DO-NOT-MERGE]" from the title?

najlkin commented 1 month ago

No, GLVis/data#3 must be merged first.

najlkin commented 1 month ago

I mean GLVis/data#3 is trivial, just needs to be synchronized with this one 😉 . So please review this and with approvals we:

  1. merge GLVis/data#3
  2. update the hash here for the merge commit
  3. remove the label do-not-merge
  4. merge this
tzanio commented 1 month ago

OK, I approve it 👍

v-dobrev commented 1 month ago

Here are some observations about the speed of the current auto-refinement choice (on Macbook pro with M2 Max chip):

tzanio commented 1 month ago

Checklist based on https://github.com/GLVis/glvis/pull/285#issuecomment-2253445609