[All Platforms] Rework Window Handle

Reporter: time-killer-games  |  Status: closed  |  Last Modified: April 27, 2020, 08:37:06 PM

This gives extensions (i.e. *.dll, *.dylib, *.so, etc.) and shell scripts (i.e. execute_shell_for_output, execute_shell, execute_program, etc.) access to window handle on all platforms except Android. Android threw errors for unknown types being used for the android window and surface, which is odd because I'm using the exact types specified by SDL docs; i guess i need to include a proper header but I don't know what that is and I've already tried the header I thought i needed but it didn't work out. so anyway, I commented out android's window handle code as a placeholder file for whenever we do add it correctly. As for the other platforms, this pr also makes the window handle more universal, expecting to use the same variable names for the window handle, and related platform-specific things such as an HINSTANCE for Windows (enigma::hInstance), and a X Window System Display * for Linux (engima::x11::disp), and these variable names are shared across the original platforms as well as their SDL equivalents. So, please do not change these variable namess one place, without changing them everywhere else they are used in the engine.

A follow up pr will need to implement external_call/define/free() on all non-Windows platforms, as well as fix the currently broken Windows implementation; this pr is in preparation for that. Also this pull request statically links SDL on all platforms.

codecov[bot]  
>Codecov Report

Merging #1854 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1854   +/-   ##
=======================================
  Coverage   28.23%   28.23%           
=======================================
  Files         166      166           
  Lines       17010    17010           
=======================================
  Hits         4803     4803           
  Misses      12207    12207           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fda6ab8...fda6ab8. Read the comment docs.

time-killer-games  

@JoshDreamland please review kthx
time-killer-games  

@RobertBColton Josh wants to know if you are ok with relicensing your libpng code you wrote under whatever SDL2 uses I think it's MIT or zlib. I want to translate this pr into a contribution to the SDL project itself, because they currently do not have a way to set an x11 icon for Linux games, and I am using both my and your code to do this, but mostly yours.
time-killer-games  

@JoshDreamland ready for review, your royal heinieness
JoshDreamland  

I thought you were going to remove the SDL integrations from Xlib?
time-killer-games  

I thought you were going to remove the SDL integrations from Xlib?

I kinda did. What you're looking at is widget related stuff, which i thought you would be ok with, because you had previously merged X11 code used in my widgets for checking the KWIN_RUNNING Atom, which would be used regardless of whether the user chose X11 or SDL.

If you want me to change it again, that's fine, but it's removing more code I took the time to write (although I understand it isn't much code or time that was spent on this to begin with, the time does add up when I also have to wait for you to review).

JoshDreamland  

I was mostly just confused about what's going on. The flow of code around window_id_init is difficult to follow, largely because it introduces a new constraint that some platforms (X11, SDL) must adhere to, but you didn't document what those constraints are, or what platforms they actually apply to.

In general, we avoid creating these couplings between systems—it leads to spaghetti code. Particularly with one or more of these systems missing from CI coverage, it's likely to cause breakages, as people won't realize that renaming a method in one system will break another. In general, healthy code means contributors can be confident that changes they make will not introduce unintended side-effects. So the less intimacy you have between systems, the better.

There are, of course, places in the engine where systems require functionality from one another. When we need all windowing systems to support a particular routine, we add it to platforms_mandatory. There are functions in that header that are similar to what you seem to want. For instance, initGameWindow is probably the place where you would have SDL stash its information somewhere. Between initGameWindow, initInput, and EnableDrawing, I suspect whatever opportunity you need to to take is covered by one of those methods.

There are other places in the engine where you'll see this kind of intimacy. The biggest are in the "Bridges" and "General" folders. Bridges depend directly on two systems, and access members between each directly to tie things together. Bridges are where you'll find code for binding libraries, like WGL and GLX, which depend on the HWND/X resource and the GL context.

We don't currently have a bridge structure to bind widget systems to window systems ("Platforms"). I am certainly not opposed to adding that bridge system, but we should do so in a way that is analogous to existing bridges, rather than tying code together between two systems.

time-killer-games  

@JoshDreamland so then, heinie, what might you suggest I do? (Also, do you mind if I call you heinie for now on?)
RobertBColton  

Yeah you can relicense my code, but not remove my name, and send the fixes upstream to SDL where they belong.
time-killer-games  

Yeah you can relicense my code, but not remove my name, and send the fixes upstream to SDL where they belong.

I wasn't planning on removing your name, this is your code mostly and that would be a rather big asshole move on my part if I did that. Thank you for the permission, that is great news!

JoshDreamland  

Is there a reason you can't move that code into the existing initGameWindow routine? Or else, invoke that routine from inside that one (so the naming convention is contained within SDL).
time-killer-games  

@JoshDreamland No, I can't just stash platform specific code in initGameWindow in SDL, it relies on X11. I set it up the way it is so that the Makefiles would ensure all that Linux-specific code doesn't attempt to be compiled anywhere but on Linux. If I put that stuff in initGameWindow, Windows and Mac wouldn't build anymore, unless I did platform macros in the code itself and not the Makefiles which I thought you guys were completely against doing.

(Yes, I'm aware Mac doesn't build regardless, but this would just further complicate that issue).

JoshDreamland  

As I said, feel free to define a helper method inside of SDL, call it from initGameWindow, and implement it differently between SDL/Win32 and SDL/X11. The distinction is that other window systems (like Xlib) don't have to worry about constraints within SDL.
time-killer-games  

@JoshDreamland ready for review
time-killer-games  

@JoshDreamland before this gets merged and you accuse me of breaking SDL. SDL has been unusable for me for a long time now, dating farther back than any of my recent pr's, including the windowing related ones obviously because I didn't touch SDL's code in those pr's at all. On Windows, the SDL window is always a blank black window for me. On Linux, I get a window client area that looks like the grey fizz on an old TV screen. Just wanted to point out these issues before this gets merged and you blame me for something I didnt break. I just never cared to mention these bugs with SDL until it became a concern that I might get blamed for it when it wasn't me who caused it. Before this pr, I haven't touched SDL hardly at all other than for implemented joysticks half way. No one blame me. You can test the current enigma-dev master to see and verify this pr doesn't cause it. I'm the only one who has been actively contributing as of late so I don't want that to be a blind excuse for any false accusations. SDL should be trashed honestly. Here's what happens on Linux when you drag a window over SDL's window:

Screenshot_20191014_002525

As you can see, SDL is already broken and can't be used, and it has been that way for a while. I find refusing to merge this pull request which has changes made that are technically correct a bad idea. I don't intend to fix things that I don't know how to fix in this pr that were already broken; it would be in your best interest to go ahead and merge this when the CI passes this time. Progress is progress, Refusing to merge this for reasons that don't make sense is counter productive and not progress.

At least from my testing on Windows just now I was able to verify Direct Sound and SDL both work together just fine with my code changes. No surprise. It's the graphics that are broken, which I am not responsible for. No surprise. (NONE of the graphics systems work with SDL btw, not just D3D).

I don't understand why we have SDL now that fundies is not helping. This means Mac and Android will never get finished, and I thought these two platforms were the only reason we ever supported SDL.

time-killer-games  

@RobertBColton it turns out me and Josh were wrong, SDL does in fact have icon support for linux already. Does this mean we should set the license back to GPL? I was going to create a GameMaker extension which uses the code and I shared it on the GameMaker Community in my code snippet topic - https://forum.yoyogames.com/index.php?threads/code-bank-share-useful-code.60575/#post-406540 - Please let me know if you want me to take this down, since the original reason we relicensed it under zlib has been nullified.
RobertBColton  

It's ok, I don't mind.
time-killer-games  

It's ok, I don't mind.

Thank you!

time-killer-games  

@RobertBColton Josh doesn't want to merge this until people other than myself have tested it on all platforms. But especially Win32. I was wondering if you'd be willing to test it on Win32 and SDL on Windows and check for regressions because apparently Josh doesn't trust me and thinks I'm lying to him when I told him there are no new regressions that I could find.
RobertBColton  

No, there's a CI testing harness for this purpose, just automate the test. Ask Josh to explain how to include a bitmap in your window test case.
time-killer-games  

@RobertBColton you don't understand, this makes a lot of changes other than just adding icon support. I also moved around a lot of the window handle code to be cross-platform and more efficient, and Josh is worried I broke Direct Sound and Direct3D for reasons I know are invalid because I took his concerns into consideration when I made these changes but he doesn't believe me. I even tested those systems to prove it. So what he concluded is unless like 10 people test this manually he ain't merging it. If you don't want to or don't have the time, I can't fault you for that. It's ok. There are other people who might be willing to do it anyway.
RobertBColton  

I'm sorry, but you will continue to elicit that reaction from Josh until you stop conflating unrelated changes together. It's the contributor's responsibility to make their changes digestible for the reviewer.
time-killer-games  

@RobertBColton and it's Josh's responsibility to not react that way because it's extremely childish, and your support for him doing this is mind blowing. I already told you you don't need to help so fuck off. Again, the only reason it isn't "digestible" is because Josh is lazy as fuck. If he adopts this attitude for any large pr's he has no business leading this project's direction.
RobertBColton  

I'm sorry, but I was just giving thoughtful advice on how to get Josh to merge the changes you want. It's the same advice I've given countless times. If it's the case that you didn't want my honest advice, please do not notify me with the at mention again in the future. Thanks!
time-killer-games  

@RobertBColton This pr was made a full month ago. I can apply your advice to my future pr's. I can't do that with an existing one. I didn't ask for any advice anyway. I just nicely asked whether you could test it and instead of telling me yes or no you bitched at me over things unrelated to what I was asking.

If you really were sorry you wouldn't get so upset with me and change the subject in provoking ways just because I kindly asked if you could test it, which I stressed you had zero obligation to do. Thanks!

You told me the answer was no, and I said that was fine. I'm not sure what else there is to discuss.

time-killer-games  

@JoshDreamland another two months of this pr has been sitting around with no review. I know you do not like the unrelated issues being solved in one mutual pr. I do not intend to do that anymore going forward, but if you made time to review this, a lot of people would appreciate it. I don't know whether Linux developers care about these things, but I know no sane Windows or Mac developer would even consider publishing a single game or app without a custom icon and would strongly prefer to not use the OS default, which makes a rather poor reflection on the developer, and gives a lazy vibe. The other things this pr fixes and adds to are rather important as well. I hope we get this sorted out soon.
JoshDreamland  

It's not about how many issues are fixed, Sam; fixing issues is fine. It's about the certifiable correctness of a change. I can certify that your change to X11's makefile is correct; it's a direct improvement, as it removes assumptions we make about libx11 and its dependencies. While I am not well-poised to verify the changes you made to DirectSound, your argument there is compelling; the removal of the dependency on window_handle() being called is an improvement, if that was actually decoupled.

Unfortunately, now DirectSound depends directly on enigma::hWnd, which could change in the future. The Bridges/ folder is the place we define things we consider to be part of the API of the system—notice that DirectSound was including Bridges/Win32/WINDOWShandle.h previously, but now it's including WINDOWSmain directly. This interdependency makes it difficult to make changes to systems in the future, and it's needless in this case because window_handle was already part of our API (and a required part of the EDL API).

Additionally, this is the third PR you have open with a copy of bitmap processing logic in it. How many copies of bitmap processing logic do we need in the engine?

time-killer-games  

@JoshDreamland thanks for the explanations. I'll have a look at this and will start working on it probably tmr. For now I want to focus on what we were discussing on hangouts because its code I'm more familiar with (because I've worked on it more recently) but yeah I agree this with the changes you suggested and that should make things more concise.
time-killer-games  

@JoshDreamland check the messages I sent you on hangouts for more details on why I requested another review without addressing everything you mentioned.
time-killer-games  

@JoshDreamland I have addressed all of your concerns in this pr. I have moved the bitmap color conversion logic to image_formats.cpp, I have reverted to the use of the compatibility switcher for window_handle(), and we are no longer including WINDOWSmain.h in DirectSound, as I have reverted it to WINDOWShandle.h. Any other reasons remaining why you should critique this, or can we finally merge this bad boy?
Please sign in to post comments, or you can view this issue on GitHub.