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

Fixes for glvis-js #310

Merged
merged 5 commits into from
Aug 20, 2024
Merged

Fixes for glvis-js #310

merged 5 commits into from
Aug 20, 2024

Conversation

justinlaughlin
Copy link
Contributor

Some of the changes in v4.3 broke portions of aux_js.cpp because they aren't using the updated constructors, enums, etc. This PR makes updates so we can build glvis-js again.

lib/aux_js.cpp Outdated Show resolved Hide resolved
@najlkin
Copy link
Contributor

najlkin commented Aug 15, 2024

Btw, how is this compiled? And another one, how about adding support for quadrature functions? 🤔

@justinlaughlin
Copy link
Contributor Author

justinlaughlin commented Aug 16, 2024

Btw, how is this compiled? And another one, how about adding support for quadrature functions? 🤔

Basically, follow the build section in the README: https://github.com/GLVis/glvis-js

Although, in order to build I needed to make a change in mfem (PR) and also use an older emscripten version. I want to setup CI similar to mfem->pymfem so it will rebuild glvis-js on pushes to glvis master 😅

Quadrature support sounds good! Let's make it a separate PR though so this can get into master 😄

@justinlaughlin justinlaughlin marked this pull request as ready for review August 16, 2024 03:00
@najlkin
Copy link
Contributor

najlkin commented Aug 16, 2024

Omg, that is complicated, ok I trust you it can be build 😉 . That CI sounds great. We may also wait for it and merge it here (like a dependent PR, keeping it open) 🚀 .
That quadrature support I was partially mentioning because it uses that coarse mesh, so there would be no need for the new/old constructors 🤔 . If you do not want it here, it is ok, we may add it temporarily. Just please add some comment to them like "used only for backward compatibility with glvis-js".

@justinlaughlin
Copy link
Contributor Author

@najlkin Is it okay to always pass StreamState.mesh_quad e.g. like in

vs = vss = new VisualizationSceneSolution(*stream_state.mesh, stream_state.sol,
                                          stream_state.mesh_quad.get(), &stream_state.normals);

even if we aren't using quadrature visualization? If so then I can remove the new constructors.

@najlkin
Copy link
Contributor

najlkin commented Aug 20, 2024

Yes, this is how it works in glvis.cpp now. Stream state does not set the coarse mesh unless it is that quadrature function OR it is 1D, which was the bug in the other PR 😅 .

@najlkin
Copy link
Contributor

najlkin commented Aug 20, 2024

Btw, a funny thing is that I do not use that coarse mesh at all 😄 , it is only for indication that you want to visualize the coarse mesh lines or not, but it is conceptually cleaner to do it this way, because it is just an implementation detail that there is no dependency, but you could use it somehow in principle.

@justinlaughlin
Copy link
Contributor Author

Okay, sounds good to me!

@najlkin
Copy link
Contributor

najlkin commented Aug 20, 2024

Besides that, it seems to me that GLVis live should be able to load and visualize quadratures with this update automatically, since we are loading stream files directly 😮 .

@justinlaughlin
Copy link
Contributor Author

Thanks for reviewing 😎

@justinlaughlin justinlaughlin merged commit c6c1f47 into master Aug 20, 2024
10 checks passed
@justinlaughlin justinlaughlin deleted the fixes-for-glvis-js branch August 20, 2024 22:36
@tzanio tzanio mentioned this pull request Aug 20, 2024
12 tasks
@justinlaughlin justinlaughlin mentioned this pull request Aug 28, 2024
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.

3 participants