Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Renderer texture clipping no longer respects scale (SDL_SetRenderScale) #11318

Closed
AntTheAlchemist opened this issue Oct 24, 2024 · 6 comments
Closed
Assignees
Milestone

Comments

@AntTheAlchemist
Copy link
Contributor

A recent change has caused textures to be cut-off from being drawn (clipped) when a scale is being set. Seems to affecting all renderers and platforms.

SDL_SetRenderScale(renderer, 0.5f, 0.5f) will see textures only being draw on the top-left quarter of the window.

The cause of this will probably be clear to whomever made a recent commit relating to texture clipping logic / viewport.

@slouken
Copy link
Collaborator

slouken commented Oct 24, 2024

@icculus probably knows what's going on here.

@slouken slouken added this to the 3.2.0 milestone Oct 24, 2024
@AntTheAlchemist
Copy link
Contributor Author

AntTheAlchemist commented Oct 24, 2024

A little test to reproduce. SDL_RenderDebugText is SO useful!

#include <SDL3/SDL_main.h>
#include <SDL3/SDL.h>
int main(int argc, char** argv) {
	SDL_Init(SDL_INIT_VIDEO);
	SDL_Window* win = SDL_CreateWindow(nullptr, 500, 500, SDL_WINDOW_RESIZABLE | SDL_WINDOW_HIGH_PIXEL_DENSITY);
	SDL_Renderer* ren = SDL_CreateRenderer(win, nullptr);
	SDL_SetRenderVSync(ren, 1);
	SDL_SetRenderScale(ren, 0.5f, 0.5f);
	bool running = true;
	while(running) {
		SDL_PumpEvents();
		SDL_Event event;
		while (SDL_PeepEvents(&event, 1, SDL_GETEVENT, SDL_EVENT_FIRST, SDL_EVENT_LAST) == 1) {
			switch (event.type) {
			case SDL_EVENT_QUIT:
				running = false;
				break;
			}
		}
		SDL_SetRenderDrawColor(ren, 0, 0, 0, SDL_ALPHA_OPAQUE);
		SDL_RenderClear(ren);
		SDL_SetRenderDrawColor(ren, 0, 255, 0, SDL_ALPHA_OPAQUE);
		for(int r = 0; r < 500; r++) 
			SDL_RenderDebugText(ren, SDL_randf() * 1000, SDL_randf() * 1000, "*");
		SDL_RenderPresent(ren);
		SDL_Delay(100);
	}
	SDL_DestroyRenderer(ren);
	SDL_DestroyWindow(win);
	SDL_Quit();
	return 0;
}

@AntTheAlchemist
Copy link
Contributor Author

Ah, yes. ef758d0 broke things for me.

@icculus icculus self-assigned this Oct 24, 2024
@icculus icculus added the icculus-priority Things @icculus is looking at within a larger milestone. label Jan 8, 2025
@icculus
Copy link
Collaborator

icculus commented Jan 8, 2025

Okay, so what's happening here is two things:

  • SDL_SetRenderScale is (currently) affecting a bunch of stuff it shouldn't: cliprect, viewport, event coordinate conversion. But it should probably only be used by primitive drawing functions like SDL_RenderLines and SDL_RenderTexture, etc. The thing that should affect all that stuff globally is logical presentation and not scale (although both apply and are multiplied together for a final scale when drawing primitives). SDL_RenderSetScale should be more like one-off calls to bump a single texture draw larger and then it gets set to something else for the next draw call. The current documentation is vague about this in some places and outright conflicting in others and should be corrected (unless I'm totally wrong about the intention here).
  • SDL_RenderTexture doesn't take the scale into account when deciding to dump a draw that's totally off-screen. I can fix that, but I'd argue that all the lower level systems (either the backends, the GPU drivers, or the GPU itself) need to have these checks anyhow, so let's let them drop things after scale is applied, and remove the check from SDL_RenderTexture completely, instead of adding more math.

I've got patches to change this, but before I push them, I just want to make sure we all agree that reducing the scope of SDL_SetRenderScale's influence is the correct and least-surprising thing to do.

@slouken
Copy link
Collaborator

slouken commented Jan 8, 2025

  • SDL_SetRenderScale is (currently) affecting a bunch of stuff it shouldn't: cliprect, viewport, event coordinate conversion. But it should probably only be used by primitive drawing functions like SDL_RenderLines and SDL_RenderTexture, etc. The thing that should affect all that stuff globally is logical presentation and not scale (although both apply and are multiplied together for a final scale when drawing primitives). SDL_RenderSetScale should be more like one-off calls to bump a single texture draw larger and then it gets set to something else for the next draw call. The current documentation is vague about this in some places and outright conflicting in others and should be corrected (unless I'm totally wrong about the intention here).

In SDL2 it affects everything because it was used to implement logical presentation, which virtualizes all your drawing. I think that's still the intention in case applications want to use it that way for their own custom logical presentation (and makes it possible for sdl2-compat to do the right thing).

  • SDL_RenderTexture doesn't take the scale into account when deciding to dump a draw that's totally off-screen. I can fix that, but I'd argue that all the lower level systems (either the backends, the GPU drivers, or the GPU itself) need to have these checks anyhow, so let's let them drop things after scale is applied, and remove the check from SDL_RenderTexture completely, instead of adding more math.

That seems reasonable. The GPU should cull everything that's completely outside the viewport and I know we early out in the software blit code in that case.

@icculus icculus closed this as completed in bf85320 Jan 8, 2025
@icculus
Copy link
Collaborator

icculus commented Jan 8, 2025

I'm not sure I agree about SDL_RenderSetScale, but we'll leave it as-is. The other fix is in now and makes this test program work as intended.

@icculus icculus removed the icculus-priority Things @icculus is looking at within a larger milestone. label Jan 8, 2025
thatcosmonaut pushed a commit to thatcosmonaut/SDL that referenced this issue Jan 13, 2025
It didn't take scale into account, and the backends would need to do clipping
anyhow, so let the system figure that out for us at the lower level.

Fixes libsdl-org#11318.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants