search

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

Number formatting #288

Closed justinlaughlin closed 1 month ago

justinlaughlin commented 1 month ago

Addressing https://github.com/GLVis/glvis/issues/260 by adding the ability to specify number formats for the colorbar and axes.

Keybind for colorbar is Alt+c and for axes is Alt+a. Once pressed, you can specify precision, format (default, fixed, or scientific), and whether the positive sign is always shown or not.

This PR also adds prompt() in aux_vis.hpp which is a more general function for requesting user input. A default value can be specified (I think implementation could be cleaner with std::optional) as well as a validator lambda.

ex9.saved defaults

image

ex9.saved using (1, 'f', true) for colorbar and (0, 'f', false) for axes.

Setting colorbar number formatting...
Enter precision (4):  1
Enter format [(d)efault, (f)ixed, (s)cientific] (d):  f
Show sign? [(1)true, (0)false] (0):  1
Setting axes number formatting...
Enter precision (4):  0
Enter format [(d)efault, (f)ixed, (s)cientific] (d):  f
Show sign? [(1)true, (0)false] (0):  0

image

justinlaughlin commented 1 month ago

Suggestion from gm: change the input from the terminal to the glvis window. e.g. by typing 5f1 to set precision=5, format type=float, and showpos=true.

najlkin commented 1 month ago

@justinlaughlin , if you mean what I was saying, it was about streams. That could be done now 💨 . In console, I think it is ok to ask the user interactively, but you may support both :wink: . Like asking the user for the code, but when left empty (there should be a hint 😇 ), it will ask for the options interactively. In the window, it makes sense to me only when we add the popup window/overlay 🤔 .

najlkin commented 1 month ago

besides that, I would prefer C-style formatting flags like in printf. 👍

v-dobrev commented 1 month ago

Suggestion from gm: change the input from the terminal to the glvis window. e.g. by typing 5f1 to set precision=5, format type=float, and showpos=true.

I think the prompts on the terminal are much better than expecting (even experts in GLVis) to remember what keys they need to press (after the initial key, which I assume is already documented in the output from pressing h) and also do it without any visual feedback.

justinlaughlin commented 1 month ago

Thanks for the help explaining some of the glvis internals @v-dobrev! I added the option to change via stream and glvis script. NumberFormatter() also has an additional constructor for c-style strings @najlkin 👍. For inputs via stream/script that is the expected format, but when pressing alt+a / alt+c it will have the same behavior (prompts).

najlkin commented 1 month ago

Cherry-pick https://github.com/GLVis/glvis/pull/286/commits/f25e99eb03776702991b9d88ac95cb44a1282c61 here 😉

tzanio commented 1 month ago

Maybe I'm doing something wrong, but on my (LOFT) Mac I can't get the Alt combinations to work.

I think I can get, Alt+c by pressing Ctrl+option+c, but Ctrl+option+a acts like Ctrl+a.

On the other hand option+left mouse seems to work correctly according to the description of Alt+left mouse.

Can someone with a Mac confirm?

v-dobrev commented 1 month ago

@tzanio, I see the same behavior on my Mac.

v-dobrev commented 1 month ago

@tzanio, I see the same behavior on my Mac.

I'm trying to debug this. I made the following additions, so I can see what events are generated when keys are pressed and released:

diff --git a/lib/sdl.cpp b/lib/sdl.cpp
index a4262be..57a3bf9 100644
--- a/lib/sdl.cpp
+++ b/lib/sdl.cpp
@@ -403,9 +403,11 @@ void SdlWindow::mainIter()
                keep_going = true;
                break;
             case SDL_KEYDOWN:
+               cout << "Event: SDL_KEYDOWN" << endl;
                keyEvent(e.key.keysym);
                break;
             case SDL_KEYUP:
+               cout << "Event: SDL_KEYUP" << endl;
                if (e.key.keysym.sym == SDLK_LCTRL
                    || e.key.keysym.sym == SDLK_RCTRL)
                {
@@ -413,6 +415,7 @@ void SdlWindow::mainIter()
                }
                break;
             case SDL_TEXTINPUT:
+               cout << "Event: SDL_TEXTINPUT" << endl;
                keyEvent(e.text.text[0]);
                break;
             case SDL_MOUSEMOTION:

On Mac, the following is generated when I press ctrl+a:

Event: SDL_KEYDOWN
Event: SDL_KEYDOWN
Autoscale: on
Event: SDL_KEYUP
Event: SDL_KEYUP

However, when I press option+a, the output is a bit different:

Event: SDL_KEYDOWN
Event: SDL_KEYDOWN
Event: SDL_TEXTINPUT
Event: SDL_KEYUP
Event: SDL_KEYUP

Pressing a without modifier keys gives:

Event: SDL_KEYDOWN
Event: SDL_TEXTINPUT
Event: SDL_KEYUP

So events are being generated but they are not processed as expected.

@justinlaughlin, I suspect you developed this on Linux and it worked for you, so the generated evens must be different. Can you please post here what events are generated on Linux in the above cases? We can then figure out a solution that works on both platforms (Mac and Linux). After that I can also test this on Windows to see if the behavior there is different from both Mac and Linux.

v-dobrev commented 1 month ago

Here's a patch that prints more information:

diff --git a/lib/sdl.cpp b/lib/sdl.cpp
index a4262be..52bd7f0 100644
--- a/lib/sdl.cpp
+++ b/lib/sdl.cpp
@@ -403,9 +403,13 @@ void SdlWindow::mainIter()
                keep_going = true;
                break;
             case SDL_KEYDOWN:
+               cout << "Event: SDL_KEYDOWN sym=" << e.key.keysym.sym
+                    << " mod=" << e.key.keysym.mod << endl;
                keyEvent(e.key.keysym);
                break;
             case SDL_KEYUP:
+               cout << "Event: SDL_KEYUP sym=" << e.key.keysym.sym
+                    << " mod=" << e.key.keysym.mod << endl;
                if (e.key.keysym.sym == SDLK_LCTRL
                    || e.key.keysym.sym == SDLK_RCTRL)
                {
@@ -413,6 +417,13 @@ void SdlWindow::mainIter()
                }
                break;
             case SDL_TEXTINPUT:
+               cout << "Event: SDL_TEXTINPUT text[0..3]="
+                    << (int)(unsigned char)(e.text.text[0])
+                    << ' ' << (int)(unsigned char)(e.text.text[1])
+                    << ' ' << (int)(unsigned char)(e.text.text[2])
+                    << ' ' << (int)(unsigned char)(e.text.text[3])
+                    << " (as codes 0-255)"
+                    << endl;
                keyEvent(e.text.text[0]);
                break;
             case SDL_MOUSEMOTION:

With this, on Mac, I get:

tzanio commented 1 month ago

I saw some specific handling for Ctrl in lib/sdl.cpp and when I added the same for Alt thing started working for me.

Here is the patch:

diff --git a/lib/sdl.cpp b/lib/sdl.cpp
index a4262be..b920ff9 100644
--- a/lib/sdl.cpp
+++ b/lib/sdl.cpp
@@ -287,6 +287,14 @@ void SdlWindow::keyEvent(SDL_Keysym& ks)
          handled = true;
       }
    }
+   else if (altDown)
+   {
+      if (onKeyDown[ks.sym])
+      {
+         onKeyDown[ks.sym](ks.mod);
+         handled = true;
+      }
+   }
    else if (ctrlDown)
    {
       if (onKeyDown[ks.sym])
@@ -295,6 +303,10 @@ void SdlWindow::keyEvent(SDL_Keysym& ks)
          handled = true;
       }
    }
+   if (ks.sym == SDLK_RALT || ks.sym == SDLK_LALT)
+   {
+      altDown = true;
+   }
    if (ks.sym == SDLK_RCTRL || ks.sym == SDLK_LCTRL)
    {
       ctrlDown = true;
@@ -406,6 +418,11 @@ void SdlWindow::mainIter()
                keyEvent(e.key.keysym);
                break;
             case SDL_KEYUP:
+               if (e.key.keysym.sym == SDLK_LALT
+                   || e.key.keysym.sym == SDLK_RALT)
+               {
+                  altDown = false;
+               }
                if (e.key.keysym.sym == SDLK_LCTRL
                    || e.key.keysym.sym == SDLK_RCTRL)
                {
diff --git a/lib/sdl.hpp b/lib/sdl.hpp
index 6d15732..89bb1a1 100644
--- a/lib/sdl.hpp
+++ b/lib/sdl.hpp
@@ -103,6 +103,7 @@ private:
    TouchDelegate onTouchPinch{nullptr};
    TouchDelegate onTouchRotate{nullptr};

+   bool altDown{false};
    bool ctrlDown{false};

 #ifdef __EMSCRIPTEN__

@v-dobrev and @justinlaughlin -- can you confirm this works on both Mac and Linux?

If we indeed need these changes, how can we get away without them before?

v-dobrev commented 1 month ago

I have a different fix that simplifies the logic in the key event functions and works on Mac. I want to test it on Linux and Windows to make sure the simpler logic works there too.

If we indeed need these changes, how can we get away without them before?

We didn't have Alt+<key> keys before, just Alt+<mouse> events.

justinlaughlin commented 1 month ago

Here's a patch that prints more information:

With this, on Mac, I get:

  • pressing a: 
    Event: SDL_KEYDOWN sym=97 mod=0
Event: SDL_TEXTINPUT text[0..3]=97 0 0 0 (as codes 0-255)
Event: SDL_KEYUP sym=97 mod=0
  • pressing ctrl+a: 
    Event: SDL_KEYDOWN sym=1073742048 mod=64
Event: SDL_KEYDOWN sym=97 mod=64
Autoscale: on
Event: SDL_KEYUP sym=97 mod=64
Event: SDL_KEYUP sym=1073742048 mod=0

  • pressing option+a:

    Event: SDL_KEYDOWN sym=1073742050 mod=256
Event: SDL_KEYDOWN sym=97 mod=256
Event: SDL_TEXTINPUT text[0..3]=195 165 0 0 (as codes 0-255)
Event: SDL_KEYUP sym=97 mod=256
Event: SDL_KEYUP sym=1073742050 mod=0

    So option is translated to KMOD_ALT in SDL2, as expected.

  • pressing cmd+a:

    Event: SDL_KEYDOWN sym=1073742051 mod=1024
Event: SDL_KEYDOWN sym=97 mod=1024
Event: SDL_KEYUP sym=97 mod=1024
Event: SDL_KEYUP sym=1073742051 mod=0

    So cmd is translated to KMOD_GUI in SDL2.

  • pressing ctrl+option+a: 
    Event: SDL_KEYDOWN sym=1073742048 mod=64
Event: SDL_KEYDOWN sym=1073742050 mod=320
Event: SDL_KEYDOWN sym=97 mod=320
Autoscale: value
Event: SDL_KEYUP sym=97 mod=320
Event: SDL_KEYUP sym=1073742050 mod=64
Event: SDL_KEYUP sym=1073742048 mod=0

Overall, it looks like there is no SDL_TEXTINPUT event when ctrl or cmd is pressed.

On WSL (Ubuntu 20.04)

tzanio commented 1 month ago

@justinlaughlin, were you able to try https://github.com/GLVis/glvis/pull/288#issuecomment-2240804015?

@v-dobrev, can you share your alternative fix?

justinlaughlin commented 1 month ago

I saw some specific handling for Ctrl in lib/sdl.cpp and when I added the same for Alt thing started working for me.

Here is the patch:

@v-dobrev and @justinlaughlin -- can you confirm this works on both Mac and Linux?

If we indeed need these changes, how can we get away without them before?

@tzanio For me on linux, this causes an additional trigger of a after terminal input is done. Meaning the axes get toggled. e.g. 

Event: SDL_KEYDOWN sym=1073742050 mod=4352
Event: SDL_KEYDOWN sym=97 mod=4352
Setting axes number formatting...
Enter precision (4):  4
Enter format [(d)efault, (f)ixed, (s)cientific] (d):  f
Show sign? [(1)true, (0)false] (0):  1
Event: SDL_TEXTINPUT text[0..3]=97 0 0 0 (as codes 0-255)
Toggling axes
Event: SDL_KEYUP sym=97 mod=4352
Event: SDL_KEYUP sym=1073742050 mod=4096

It looks like SDL_TEXTINPUT is what is making the additional call (keyEvent(e.text.text[0]);) but I'm not sure yet how to fix it.

v-dobrev commented 1 month ago

@justinlaughlin, @tzanio, I created a PR (#294) towards this PR with my proposed changes in the key events handling. Take a look and try it.

justinlaughlin commented 1 month ago

@justinlaughlin, @tzanio, I created a PR (#294) towards this PR with my proposed changes in the key events handling. Take a look and try it.

Excellent! I just tried this and it is working.

tzanio commented 1 month ago

@justinlaughlin, @tzanio, I created a PR (#294) towards this PR with my proposed changes in the key events handling. Take a look and try it.

Thanks @v-dobrev, I can confirm #294 works on Mac too!

tzanio commented 1 month ago

@v-dobrev, should we mention the sld.?pp changes in CHANGELOG?

v-dobrev commented 1 month ago

@v-dobrev, should we mention the sld.?pp changes in CHANGELOG?

It is something internal, so I'm not sure it is important to mention it there.

v-dobrev commented 1 month ago

This PR need a new baseline screenshot for one test -- stream_mesh-explorer. This change will be independent of the baseline changes for #285, as prepared in https://github.com/GLVis/data/pull/3. We should:

  1. prepare an update for https://github.com/GLVis/data,
  2. test it here (update the submodule hash),
  3. when the tests are fixed, merge the update for https://github.com/GLVis/data
  4. update the submodule hash with the merged hash of https://github.com/GLVis/data
  5. merge this PR.

Does this sound ok?

v-dobrev commented 1 month ago

I did 1 and 2 from the above list. Waiting on tests to see if there are any issues.

tzanio commented 1 month ago

Does this sound ok?

Yes

justinlaughlin commented 1 month ago

Tests pass :). Can we merge? It'd probably be simpler to merge this prior to merging https://github.com/GLVis/glvis/pull/285 since that PR has different baselines.

tzanio commented 1 month ago

Go ahead and merge @justinlaughlin.

justinlaughlin commented 1 month ago

Go ahead and merge @justinlaughlin.

I don't have the power to 😅

tzanio commented 1 month ago

Go ahead and merge @justinlaughlin.

I don't have the power to 😅

try again

najlkin commented 1 month ago

@justinlaughlin , the merge does not pass on master through make style check! 😱 I do not know why PR CI does not have those checks, but this must be fixed asap as it is failing in autotests!