Made arguments struct within SDL2application take in (and store) argc by value instead of reference

[Ben-Bingham]

I think this must be left over from a previous version of the software or something, it seems really weird that this struct needs to hold onto argc as a reference, and why it cant just copy it in, its one int, and I have no idea why it would change.

[mosra]

Hi, the reason for this was to give the library / engine code a chance to filter the arguments. So for example it'd consume arguments it recognizes, removes them from the argv array, and updates argc accordingly. The application code then sees just what wasn't intended for the engine.

Qt, for example, does the same.

At the moment however, nothing from the library actually modifies the argument list, because Utility::Arguments is made in a way that allows several argument parsers to coexist, and so it wasn't deemed neccessary. That can eventually change tho.

Is this causing some problems on your end? If so, what's your use case? Or were you just curious why it was done this way?

[Ben-Bingham]

I'm working on a project where argc and argv are passed into a few functions before they are passed to magnum within a lambda meaning that every function that passed argc and v to the next one needs to take them in by reference instead of copying which would be simpler. Its not that big of a deal, I honestly just thought that argc being a reference was a mistake as opposed to inentional.

[mosra]

Interesting. The assumption I made on my end was that the application class would be always the most top-level thing in main(), which means the code wouldn't even need to notice that argc gets passed in by a reference.

But if you have a bunch of functions wrapping that, I can see how that can get annoying, yeah.

The arguments are not critically important, so if the application code itself doesn't need to parse anything from the command line, there's just the --magnum-* arguments left. Which in a happy scenario you wouldn't even need to use, and on desktop platforms at least they can all be expressed through an env var as well. So you could have some "global" int argc = 0 that you use in this particular case instead. All internals should work with argc being 0 and they wouldn't have a reason to modify it in that case either, so it should work even in a multi-threaded scenario.

I could also add a constructor overload that doesn't take the argc/argv at all, but I'm hesitating to do that because then people would use that constructor implicitly because it's easier. Which would mean the magnum-based apps would no longer recognize the --magnum-* options that become really useful when debugging, and specifying them via an env var isn't always possible, such as on the web.

[jonesmz]

I suppose this raises the question of what negative harm there would be if the arguments weren't mutated by the application class.

Having a few extra entries that are namespaces shouldn't hurt code that understands it may have to discard unknown arcs.

[mosra]

Good point.

If I would be designing the Application classes today, I probably wouldn't make it like Qt with int& argc, because Utility::Arguments can already handle that namespacing on its own, and on the other hand having the engine internals possibly take some unspecified arguments off the command line feels kind of suspicious and prone to conflicts / clashes with arguments the application code uses.

But back when I designed this in 2010, I thought int& argc was a neat idea, didn't know any better alternative, and now I'm bound by backwards compatibility. There may be existing code that's actually making use of this mutability in some way, and if I'd remove that feature, it'd break. I'm open to changing this eventually, but need to think of the consequences first.

[jonesmz]

I guess if you don't think the backwards compat can be addressed. There's always SDL3, which is now available.

So instead of fixing it in the sdl2application class, you apply the fix to the sdl3application class.

I'd really like to use sdl3 over 2, because they fixed an issue with being able to compile with mingw and I've been wanting to add mingw to my CI for ages.

[mosra]

Yes, but I want to have the behavior consistent between Sdl*Application, EmscriptenApplication and GlfwApplication, so if I change it in one, I should do it for the others as well, otherwise code that switches between them depending on platform would break.

Switching between SDL and GLFW isn't all that common, but EmscriptenApplication on the web is a commonly preferred choice over SDL.

(Sorry if it sounds like I'm just piling up reasons why I can't change anything, that's not the intent. Just explaining the background.)