WIP: Use scaled size for mouse events in debug windows

[JonathanMurray]

The tile management in debug windows (hovering and clicking on squares) was not working correctly after the window was scaled. The logic for determining the tile index was relying on the original scaling of the window, resulting in the incorrect tile being targetted.

By listening for SDL window resize events, we can keep the scaling value up-to-date. We also need to change the one-dimensional scaling value to separate horizontal/vertical scaling due to how the resizing allows the aspect ratio to change.

Checklist for pull-requests

1. The project is licensed under LGPL (see LICENSE.md). When merged, your code will be under the same license.
   So make sure you have read and understand it.
2. Please coordinate with one of the core developers before making a big pull-request.
   It's a shame to make something big that doesn't fit the project.
3. Remember to make a separate branch on your fork. This makes it a lot easier for the core developers to help
   getting your pull-request ready.
4. Install pip install pre-commit. This takes care of the formatting rules when you commit your code.
5. Add tests. We need good pytests for your code. This will help us keep the project stable.
6. Please don't change the code style, unless it's specifically asked for.

-->

[JonathanMurray]

Heya! First time PR. Let me know if there's some obvious step I missed in the contribution process :)

I tried out the debug mode of the emulator which seems really cool and useful, but I noticed that the mouse events don't work as expected after I scale the windows. I was a bit surprised that the windows even allowed scaling, but that seems intended judging from the code.

For fun and practice I decided to try and resolve the issue, and this is what I arrived at. Since the code-base is new to me, I may be missing something obvious or perhaps accidentally breaking something that I'm not aware of, so I'm marking the PR as WIP to reflect that.

Do you think the gist of the PR seems to be right approach to solving the problem?

[JonathanMurray]

I noticed that some event IDs are prefixed with _INTERNAL_ but wasn't sure if the resize event is also to be considered as such. Does "internal" refer to the fact that the event comes from the framework itself and shouldn't be possible to trigger from client code? If so, I would suppose window resizing counts as internal.

[Baekalfen]

Yes, internal ones, are not supposed to be called from user-code. I guess there are a few we should add _INTERNAL to.

[JonathanMurray]

Cool! I'll change this new event to _INTERNAL then. (At least I cannot think of a use-case where this would be called from user-code.)

[JonathanMurray]

I do notice that there's already a field called _scaledresolution which seems to be updated in _glreshape (but not when using SDL2 windows, from what I can tell). Perhaps a better approach than what I have now would be to leave scale alone and set _scaledresolution from the SDL2 code.

[Baekalfen]

Thanks for your PR! I'll get back to you as soon as I have some time to properly review your code

[krs013]

This is great! As far as I can see the changes are good, though we'll want to confirm that the new window event is integrated the right way.

However, if we start paying attention to resize events, maybe we should just lock the aspect ratio, instead of separating the scale into different numbers? It doesn't really make a lot of sense to have the window stretched, in my opinion. I would rather either set the window size to lock to the correct ratio, or add letterboxes.

[Baekalfen]

Thanks for your PR, really well done! I was wondering, if it is possible/easier to poll the window dimension instead?

[Baekalfen]

Yes, internal ones, are not supposed to be called from user-code. I guess there are a few we should add _INTERNAL to.

[Baekalfen]

However, if we start paying attention to resize events, maybe we should just lock the aspect ratio

I would very much like to support locked aspect ratio, but when I experimented with it, it was surprisingly hard. When I looked around for examples, there are actually very few apps that lock the window aspect ratio ― only media players come to mind.

[JonathanMurray]

I tried something now for locking the aspect ratio, using SDL_RenderSetLogicalSize. I'll push an updated version.

I was wondering, if it is possible/easier to poll the window dimension instead?

Hm, yes that's an interesting option I think! Sounds like it would require less complex code. Perhaps if we don't care about maintaining aspect ratio for now, we should explore that further? For maintaining aspect-ratio I'm guessing the window resize events are needed though.

[JonathanMurray]

One thing that's clearly missing from the PR is OpenGL support. I'm thinking that something could perhaps be added to _glreshape, but I'm having trouble running the app with OpenGL at all on my Macbook.

[krs013]

Unless it's a Big Sur/M1 thing, it should be enough to install XQuartz and brew install freeglut to get OpenGL working, I think.

[JonathanMurray]

I assumed it was a Big Sur thing, based on this: https://stackoverflow.com/questions/63475461/unable-to-import-opengl-gl-in-python-on-macos. However, installing XQuartz and freeglut did bring me a bit further, but now I'm crashing after a second or so (instead of failing on OpenGL imports) on what I understand to be a multithreading issue (Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'NSScreen reconfig must only happen on the main thread.' ~ stevenlovegrove/Pangolin#111 (comment))

Anyway, reading the code a bit more carefully now (BaseDebugWindow) I am starting to believe that OpenGL is not involved at all in the handling/rendering of the debug windows (?). In that case, I suppose OpenGL support may not be a concern for the debug scaling change.

[krs013]

Hmm yeah the OpenGL window isn't really intended to run alongside the debug window. I don't know if it had even been tried before you brought it up. Having just tested it, it looks like it actually does work on the master branch, but I get the same exception as you when trying to run it on your branch.

If we can get this to go away, great, but it not I'm personally fine with saying "don't use OpenGL and debug at the same time." OpenGL seems to still work fine on its own.

Also, I see you got the letterboxing working! Super awesome. I was a bit surprised at first to see you changing the LogicalRender size but it seems to be a good solution for locking the aspect ratio--probably worth doing to the main window as well IMO. I want to take a closer look at how the LogicalRenderSize works, though, because it goes against my understanding of how our rendering works, and I need to see if I misunderstood its role or if we're doing something wrong here. So I'll look at that, but in the meantime, great work!

[Baekalfen]

Are you two able to launch the debugger along with the OpenGL window? There should be a switch to force SDL2 if debug is enabled

[Baekalfen]

Can't find it right now, but there used to be such limiter.

[JonathanMurray]

I was a bit surprised at first to see you changing the LogicalRender size [...] I want to take a closer look at how the LogicalRenderSize works, though, because it goes against my understanding of how our rendering works, and I need to see if I misunderstood its role or if we're doing something wrong here. So I'll look at that

Sounds good! I haven't worked directly with SDL before, so I don't know if this approach could be considered idiomatic and/or sensible.

[Baekalfen]

I had a better look, and we do actually kinda support running OpenGL along with the debugger now. I think I changed it after refactoring to plugins, as it felt silly to block something that more or less works.

EDIT: But just to clarify, the debug windows are using SDL2 regardless of choosing OpenGL for the main window.

[krs013]

Alright, so I've been looking over the source for the SDL Renderer for a while now and it turns out that it actually scales the mouse events for you if you set a logical size! So I feel a little bad saying this after all the work you've done, but it seems that all we need to do is to set the LogicalRenderSize to the original (unscaled) size when we create the renderers, and remove all of our current logic for converting coordinates using the saved scales, and then it just works. No need for window resize events, or letterbox logic... it's all taken care with one line! We wouldn't even need to track the scale variable except for the logic we use to lay out the windows.

So... I'm not sure what to do now. Technically this PR isn't needed after implementing that fix. Do we want window resize events for any other reason?

[JonathanMurray]

Ah! I've completely missed your comments. If there's a one-liner that takes care of everything, that's definitely the way to go 😄

Feel free to close this PR if it turns out that way! I can perhaps take a look at it also, unless someone else is already taking care of it.

[JonathanMurray]

@krs013 Quite a bit simpler indeed #199

[Baekalfen]

Closed in favor of #199