Wrong SDL_RenderGetClipRect/SDL_RenderSetClipRect behaviour

Hi there!
After upgrading from 2.0.12 to 2.0.22 I got a strange glitch.
After some investigation, I wrote simple code to reproduce the error.

    float scale = float(2048) / float(1920); //window size.x = 2048, logical size.x =1920
    SDL_RenderSetScale(renderer, scale, scale);
    SDL_Rect clip = { 0, 0, 1920, 1080 };
    SDL_RenderSetClipRect(renderer, &clip);
    SDL_Rect new_clip;
    SDL_RenderGetClipRect(renderer, &new_clip);
    printf("before: %d %d %d %d\n", clip.x, clip.y, clip.w, clip.h);
    printf("after : %d %d %d %d\n", new_clip.x, new_clip.y, new_clip.w, new_clip.h);

prints

before: 0 0 1920 1080
after : 0 0 1919 1080

Problem occurs because SDL_RenderGetClipRect uses SDL_floor for calculating dimensions (w/h).
Is it a bug or expected behavior?

@rmg.nik
Issue was added here Wrong SDL_RenderGetClipRect/SDL_RenderSetClipRect behaviour · Issue #5547 · libsdl-org/SDL · GitHub

1 Like

@rmg.nik
Could you test the fix that’s been merged ?
also this is not a regression: this was already present in 2.0.12 and before. So Not sur if this fix the glitch

I tested and the problem is still reproducing.

I extended the code to reproduce the problem. It is “simple draw loop”. I don’t remember why I added logic to backuping the scale and restoring the cliprect for the screen clearing, but if remove the scale backuping-restoring the bug disappears.

    //init scale and cliprect
    float scale = float(2048) / float(1920); //window size.x = 2048, logical size.x =1920
    SDL_RenderSetScale(renderer, scale, scale);
    SDL_Rect clip = { 0, 0, 1920, 1080 };
    SDL_RenderSetClipRect(renderer, &clip);
    for (int i = 0; i < 1000; ++i)
    {
        //----------------------------------------
        //clear the entire screen logic
        SDL_Rect cliprect;
        float sx, sy;                                //remove this line to fix the problem

        SDL_RenderGetScale(renderer, &sx, &sy);      //remove this line to fix the problem
        SDL_RenderSetScale(renderer, 1.0f, 1.0f);    //remove this line to fix the problem
        SDL_RenderGetClipRect(renderer, &cliprect);
        SDL_RenderSetClipRect(renderer, nullptr);
        //SDL_RenderClear(renderer);
        SDL_RenderSetClipRect(renderer, &cliprect);
        SDL_RenderSetScale(renderer, sx, sy);        //remove this line to fix the problem

        //----------------------------------------
        //backup cliprect
        SDL_Rect bac_cliprect;
        SDL_RenderGetClipRect(renderer, &bac_cliprect);

        //----------------------------------------
        //set smaller cliprect
        //this step is unnecessary to reproduce the problem
        SDL_Rect sub_cliprect;
        sub_cliprect.x = 0;
        sub_cliprect.y = 0;
        sub_cliprect.w = 100;
        sub_cliprect.h = 100;
        SDL_RenderSetClipRect(renderer, &sub_cliprect);

        //draw something logic
        
        //----------------------------------------
        //restore cliprect
        SDL_RenderSetClipRect(renderer, &bac_cliprect);

        //----------------------------------------
        //print current cliprect
        SDL_RenderGetClipRect(renderer, &cliprect);
        printf("cliprect: %d %d %d %d\n", cliprect.x, cliprect.y, cliprect.w, cliprect.h);
    }

And no - in 2.0.12 the logic was slightly different and bug was missing.
So, there is no problem in float precision. Here or a problem in my logic, or it is necessary to use SDL_roundf in SDL_RenderGetClipRect.

Can you re-try exactly your first test-code ? I’ve doubled-checked and

before: 0 0 1920 1080
after : 0 0 1919 1080

was the output for every previous SDL version I have. And now in head :

before: 0 0 1920 1080
after : 0 0 1920 1080

With your second code, this is different.
before the output was constant to:
cliprect: 0 0 1919 1080
( which was also incorrect, because you expected 1920, where you get 1919).
and that has also changed with 2.0.16. and also later.

When you get and re-set the cliprect, you in fact apply the rounding method.

diff --git a/src/render/SDL_render.c b/src/render/SDL_render.c
index 58cda3d60..e4f50e9da 100644
--- a/src/render/SDL_render.c
+++ b/src/render/SDL_render.c
@@ -2464,10 +2464,10 @@ SDL_RenderGetViewport(SDL_Renderer * renderer, SDL_Rect * rect)
     CHECK_RENDERER_MAGIC(renderer, );
 
     if (rect) {
-        rect->x = (int)SDL_floor(renderer->viewport.x / renderer->scale.x);
-        rect->y = (int)SDL_floor(renderer->viewport.y / renderer->scale.y);
-        rect->w = (int)SDL_floor(renderer->viewport.w / renderer->scale.x);
-        rect->h = (int)SDL_floor(renderer->viewport.h / renderer->scale.y);
+        rect->x = (int)SDL_round(renderer->viewport.x / renderer->scale.x);
+        rect->y = (int)SDL_round(renderer->viewport.y / renderer->scale.y);
+        rect->w = (int)SDL_round(renderer->viewport.w / renderer->scale.x);
+        rect->h = (int)SDL_round(renderer->viewport.h / renderer->scale.y);
     }
 }
 
@@ -2507,10 +2507,10 @@ SDL_RenderGetClipRect(SDL_Renderer * renderer, SDL_Rect * rect)
     CHECK_RENDERER_MAGIC(renderer, )
 
     if (rect) {
-        rect->x = (int)SDL_floor(renderer->clip_rect.x / renderer->scale.x);
-        rect->y = (int)SDL_floor(renderer->clip_rect.y / renderer->scale.y);
-        rect->w = (int)SDL_floor(renderer->clip_rect.w / renderer->scale.x);
-        rect->h = (int)SDL_floor(renderer->clip_rect.h / renderer->scale.y);
+        rect->x = (int)SDL_round(renderer->clip_rect.x / renderer->scale.x);
+        rect->y = (int)SDL_round(renderer->clip_rect.y / renderer->scale.y);
+        rect->w = (int)SDL_round(renderer->clip_rect.w / renderer->scale.x);
+        rect->h = (int)SDL_round(renderer->clip_rect.h / renderer->scale.y);
     }
 }

Changing the rounding method would fit what you expect.
(Since, cliprect and viewport use the same calculation. we need to update GetViewport,
and same thing with w/h vs x/y)

But I think this doesn’t reflect the real viewport / clip that end up being used inside the backend.
Get/Set cliprect are not usual setter / getter methods : but more like a “get the cliprect at some scale”.

Maybe we could also have the float versions that would be better for you use-case.
But even with the functions below, the second test case still produces a “1919 1080” output.
Because it single precision vs double precision.

diff --git a/src/render/SDL_render.c b/src/render/SDL_render.c
index 58cda3d60..f7899e746 100644
--- a/src/render/SDL_render.c
+++ b/src/render/SDL_render.c
@@ -2514,6 +2514,42 @@ SDL_RenderGetClipRect(SDL_Renderer * renderer, SDL_Rect * rect)
     }
 }
 
+int
+SDL_RenderSetClipRectF(SDL_Renderer * renderer, const SDL_FRect * frect)
+{
+    int retval;
+    CHECK_RENDERER_MAGIC(renderer, -1)
+
+    if (frect) {
+        renderer->clipping_enabled = SDL_TRUE;
+        renderer->clip_rect.x = (double)frect->x * renderer->scale.x;
+        renderer->clip_rect.y = (double)frect->y * renderer->scale.y;
+        renderer->clip_rect.w = (double)frect->w * renderer->scale.x;
+        renderer->clip_rect.h = (double)frect->h * renderer->scale.y;
+    } else {
+        renderer->clipping_enabled = SDL_FALSE;
+        SDL_zero(renderer->clip_rect);
+    }
+
+    retval = QueueCmdSetClipRect(renderer);
+    return retval < 0 ? retval : FlushRenderCommandsIfNotBatching(renderer);
+}
+
+void
+SDL_RenderGetClipRectF(SDL_Renderer * renderer, SDL_FRect * frect)
+{
+    CHECK_RENDERER_MAGIC(renderer, )
+
+    if (frect) {
+        frect->x = (float)(renderer->clip_rect.x / renderer->scale.x);
+        frect->y = (float)(renderer->clip_rect.y / renderer->scale.y);
+        frect->w = (float)(renderer->clip_rect.w / renderer->scale.x);
+        frect->h = (float)(renderer->clip_rect.h / renderer->scale.y);
+    }
+}
+

Yes my first code works correctly with ‘double’. Sorry I have not published a full problem…
I don’t think that extending SDL API is a good idea. I always thought that SDL should be simple).
Replacing SDL_floor with SDL_round fixed the problem.

Replacing by ‘round’ indeed fixed the test-case, but the ClipRect / Viewporit you get isn’t what’s actually applied.

Thinking again about it. I think you should do the save/restore like this :

  • Backup the ClipRect,
  • Backup the Scale.
    … do other rendering.
  • Restore the Scale
  • Restore the ClipRect

Maybe, but for now I just need that after SDL_RenderSetClipRect SDL_RenderGetClipRect returns the same value. This we achieve by replacing SDL_floor with SDL_round.
Thank you!

You’re talking about modifying your own sdl version to work around temporarly ?

Yes, I modified the SDL source code in our project.