Add Renderer.logical_to_window and Renderer.window_to_logical from SDL2

[DickerDackel]

See https://wiki.libsdl.org/SDL2/SDL_RenderLogicalToWindow and https://wiki.libsdl.org/SDL2/SDL_RenderWindowToLogical

When setting renderer.logical_size, these two functions offer translation between window space and logical space when the renderer is scaled different from the window.

This is e.g. useful to translate mouse coordinates that are in window space, to logical_size coordinates of the renderer (or back).

[DickerDackel]

I've seen the failing stub stuff, have no experience with it, but just had a look at the github action. Fix coming...

[ankith26]

On the SDL3 porting guide I found this note

SDL_RenderWindowToLogical() and SDL_RenderLogicalToWindow() have been renamed SDL_RenderCoordinatesFromWindow() and SDL_RenderCoordinatesToWindow() and take floating point coordinates in both directions.

I think we should consider whether we should stick to SDL2 terminology or use SDL3's. In either case, I think it makes more sense to keep input consistent for both methods (a single Point argument, like rest of the functions in this module) and a tuple[float, float] return type.

[DickerDackel]

I think we should consider whether we should stick to SDL2 terminology or use SDL3's.

I've not yet been in this situation, so this is unfounded gut feeling, but if I was using the _sdl2 API, I'd look up the SDL2 docs.

How about aliasing them to provide both names? Like Vector2's "lengh" and "magnitude" methods?

In either case, I think it makes more sense to keep input consistent for both methods (a single Point argument, like rest of the functions in this module) and a tuple[float, float] return type.

Sure, I'll change that. Give me an hour or so...

[DickerDackel]

like rest of the functions in this module) and a tuple[float, float] return type.

Looking at the other functions, e.g. draw_line, draw_point, they also all accept kwargs, which brings us to naming conventions. draw_point has a point argument, draw_line accepts p1 and p2. I'm an old guy, so I'm happy with p, will assume people would prefere the more obvious point, but will (probably) also happily implement any other suggestion.

Leave any thoughts here, I'll go with point until I hear something else, since that's pretty much what we do translate.

[ankith26]

LGTM, thanks for the PR! 🎉

But before we merge it, we should also consider whether the same functionality must be implemented in pygame._sdl2.video which is the original implementation. I haven't been keeping up with pygame._render so I don't know what the plan is, requesting @Starbuck5 and/or @MightyJosip to review this aspect.

[damusss]

I haven't been keeping up with pygame._render so I don't know what the plan is

can't assure my info is correct, but _render is a C implementation of sdl2_video. so from what I know _render (future render) is the "official", future-to-be public implementation of sdl2_video. it's like pygame.window and sdl2_video.Window, the latter (second one, I always confuse former and latter) stopped receiving updates, and in my opinion it makes sense to keep updates to _render.

[damusss]

Everything looks good to me and makes sense. not merging right away to allow more contributors to check it since it would be new api (tho not immediately public)

[MightyJosip]

LGTM. But we need to consider what to do with the docs. Because by the way you currently did it, this functionality would be available in pygame._sdl2.video.Renderer as well, however, it exists only in pygame._render. After the testing phase, it should probably be moved to another module docs, like it currently works for Window https://pyga.me/docs/ref/sdl2_video.html#pygame._sdl2.video.Window

[DickerDackel]

I followed the existing doc strings in render.c. I just also grepped the whole project directory for one of the existing doc strings, and there is no other place right now where these are referenced.

So I see three options:

1. As discussed above, create these two functions in the cython sdl2_video too.
2. create a dedicated doc include for render, but that would go against what Damus did here: d1f5a4b
3. Leave it as is, until the render module is official and then move all DOC_SDL2_VIDEO_RENDERER_* back into a dedicated doc file.

[Starbuck5]

I haven't been following the discussion here at all over the last month, but this seems like a very high quality PR! Thanks DickerDackel.

I feel a little bad coming in with naming at this point in time, but I feel like we should be using SDL3 terminology for everything going forward. One of my goals is to get us closer to SDL functionality with SDL names, and when we're actively moving to SDL3 I think there would have to be a strong argument against using their names to not use them.

[DickerDackel]

I haven't been following the discussion here at all over the last month, but this seems like a very high quality PR! Thanks DickerDackel.

I feel a little bad coming in with naming at this point in time, but I feel like we should be using SDL3 terminology for everything going forward. One of my goals is to get us closer to SDL functionality with SDL names, and when we're actively moving to SDL3 I think there would have to be a strong argument against using their names to not use them.

Sorry, I was lazy with email for the last days.

I see two options. Either change everything to the new names render_coordinates_(to|from)_window, or alias them and leave the sdl2 version in as well. Like e.g. Vector2.length() and Vector2.magnitude().

Both are quick fixes, but I'd like to get the opinion of the people that already approved as is, before changing it and then having to undo it again if there's no consensus.

[damusss]

If we want to follow SDL3 naming schemes, i believe coordinates_to_window and coordinates_from_window is a good option. The "render" part doesn't make any sense in this context (I think) and is only present to make the functions be part of the Renderer category but we don't need that since they already belong to the class.

I don't think it's a good idea to add the feature with built in aliases. I'm not fond of both length and magnitude existing as I usually prefer a cleaner API.

[DickerDackel]

I don't think it's a good idea to leave the "render_" part out, becaused even for the renderer, both kinds of coordinates are native properties. In that way, I found the sdl2 naming clearer, because it gave the type of "both ends" of the conversion.

I think, sticking with the SDL3 naming would be the most sane thing. If the goal is SDL3 naming, stick with it. If the goal is being descriptive, then leave the old names.

I'm lacking theproject history to know if there is an established standard or practice for deviating from sdl names.

Now we don't have chosen one option, but have 3 options instead...

Having let it simmer for a while the function names without render_ feel less and less awkward. Is @Starbuck5 ok with the shortening, then I'll change the naming to Damuss' suggestion.

[damusss]

@DickerDackel I know you "canceled" what you said, but to super clarify. I read RenderCoordinatesToWindow. as "Render the coordinates to the window" and that's no way what it does.
We removed "Render" from every function inside Renderer. for example Renderer.draw_point calls SDL_RenderDrawPoint. it would be weird to have Renderer.render_draw_point. They added Render to everything because that's what c devs do to have some kind of function namespace (and I'm probably speaking to an expert at such).
I also like the SDL2 namings, but the new one has some sense to it.

[DickerDackel]

We removed "Render" from every function inside Renderer. for example Renderer.draw_point calls SDL_RenderDrawPoint.

That perfectly answers the implied question from this:

I'm lacking theproject history to know if there is an established standard or practice for deviating from sdl names.

So, everybody agrees on Renderer.coordinates_(to|from)_window?

[Starbuck5]

So, everybody agrees on Renderer.coordinates_(to|from)_window?

That sounds good to me.

[ankith26]

Still looks good to me with the name change.