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

Refactoring to remove global state #170

Open
wants to merge 40 commits into
base: master
Choose a base branch
from
Open

Conversation

publixsubfan
Copy link
Contributor

This PR is intended to split off some of the work from #149 into a somewhat-smaller PR.

  • Adds a new class, GLVisWindow, which consolidates most of the global state within aux_vis.cpp. GLVisWindow handles creation of the SdlWindow, VisualizationScene*, and GLVisCommand objects from a StreamState object and/or an array of input streams.
  • Make VisualizationScene* event handlers member functions where possible, removing dependence on global pointers.
  • Remove most references to global objects from the core glvis library.
  • Move handling of multiplexing of GLVisCommand and other idle function runs from SdlWindow to GLVisWindow.

@publixsubfan publixsubfan requested review from tomstitt and v-dobrev May 12, 2021 23:33
@publixsubfan publixsubfan self-assigned this May 12, 2021
face_has_kerning(false)
face_has_kerning(false),
is_hidpi(false),
ppi_w(96), ppi_h(96)
Copy link
Member

Choose a reason for hiding this comment

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

(minor) these could be default values when the members are declared:

... etc ...
bool face_has_kerning = false;
bool is_hidpi = false;
int ppi_w = 96, ppi_h = 96;

@tomstitt
Copy link
Member

tomstitt commented Jun 2, 2021

I can now build and run the js version but there is at least on thing that isn't working yet. I saw that the CallKeySequence call was moved from aux_js.cpp:startVisualization into aux_vis.cpp:InitVisualization (which is called in aux_js.cpp:startVisualization) but it doesn't seem to work, when I open ex9 I see this:

Screen Shot 2021-06-01 at 5 47 14 PM

Instead of this:

Screen Shot 2021-06-01 at 5 47 31 PM

I'll spend some more time debugging.

Tom Stitt added 2 commits June 5, 2021 11:42
closer to working but not quite there
…is is happening yet but reusing the existing GLVisWindow does the trick (and is what we were doing previously)
@tomstitt
Copy link
Member

The JS target seems to work well for me now, I slacked a copy to @tzanio and @kanye-quest so check it out if you want

@tzanio
Copy link
Member

tzanio commented Jun 16, 2021

All other examples from glvis.org/live work for me, but I still get an error with Navier in this branch, e.g. the following run:

./glvis -saved ~/glvis-web/src/data/navier.saved

shows

Screen Shot 2021-06-15 at 5 18 24 PM

instead of

Screen Shot 2021-06-15 at 5 18 39 PM

Copy link
Member

@tzanio tzanio left a comment

Choose a reason for hiding this comment

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

Preliminary review

}

if (input_streams.Size() > 0)
catch (std::runtime_error& ex)
Copy link
Member

Choose a reason for hiding this comment

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

I think this may be the first use of exceptions in MFEM or GLVis 😁

continue;
}


Copy link
Member

Choose a reason for hiding this comment

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

one empty line

@tzanio tzanio mentioned this pull request Aug 15, 2021
15 tasks
@tzanio tzanio modified the milestones: glvis-4.1, glvis-4.2 Aug 17, 2021
@tzanio tzanio mentioned this pull request Aug 27, 2021
6 tasks
@tzanio tzanio removed this from the glvis-4.2 milestone Sep 26, 2021
@tzanio tzanio mentioned this pull request Jun 19, 2024
22 tasks
@tzanio tzanio mentioned this pull request Aug 7, 2024
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants