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

Fixed issue where the viewport's width and/or height could be off by 1 pixel #598

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@Vhab
Contributor

Vhab commented May 14, 2014

Due to float imprecision there was a bug causing a viewport to be 1 pixel smaller in width and/or height than it should be.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila May 14, 2014

Member

Rounding to the nearest integer is closer to the desired value, but it's just a convention, I wouldn't say that the current code has a bug. Or did I miss something?

When you say that the viewport is "off by 1 pixel", it's compared to what?

Member

LaurentGomila commented May 14, 2014

Rounding to the nearest integer is closer to the desired value, but it's just a convention, I wouldn't say that the current code has a bug. Or did I miss something?

When you say that the viewport is "off by 1 pixel", it's compared to what?

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch May 14, 2014

Member

The change sounds reasonable. But at the same time it's probably in the same corner as other subpixel accuracy problems. One might expect either rounding - to smaller or larger pixel, so I'd also consider it "a convention".

Member

MarioLiebisch commented May 14, 2014

The change sounds reasonable. But at the same time it's probably in the same corner as other subpixel accuracy problems. One might expect either rounding - to smaller or larger pixel, so I'd also consider it "a convention".

@Vhab

This comment has been minimized.

Show comment
Hide comment
@Vhab

Vhab May 14, 2014

Contributor

The issue is that it sometimes is "off by 1", not consistently, due to the lack of rounding.

For example, I'd calculate the values of a view for a portion of the screen like this:

int x, y, width, height;
float viewLeft = (float)x / screenWidth;
float viewTop = (float)y / screenHeight;
float viewWidth = (float)width / screenWidth;
float viewHeight = (float)height / screenHeight;

As SFML's API expects the viewport values to be given in 0-1

I noticed the bug when the contents of the view (containing sprites which were rendered with a 1 to 1 pixel mapping) were slightly blurry. But views of a different size would be fine and render as intended.
Debugging through SFML I found the view actually ended up 1 pixel smaller with certain view sizes, causing the content of size X to be rendered in a view the size of X-1, thus causing the sprites to misalign.

If we look at

return IntRect(static_cast<int>(0.5f + width  * viewport.left),
                   static_cast<int>(0.5f + height * viewport.top),
                   static_cast<int>(width  * viewport.width),
                   static_cast<int>(height * viewport.height));

We can see it basically does the inverse of above, except for the last 2 values it didn't round the values. Thus due to float imprecision, it sometimes ended up 1 pixel smaller than intended.

Contributor

Vhab commented May 14, 2014

The issue is that it sometimes is "off by 1", not consistently, due to the lack of rounding.

For example, I'd calculate the values of a view for a portion of the screen like this:

int x, y, width, height;
float viewLeft = (float)x / screenWidth;
float viewTop = (float)y / screenHeight;
float viewWidth = (float)width / screenWidth;
float viewHeight = (float)height / screenHeight;

As SFML's API expects the viewport values to be given in 0-1

I noticed the bug when the contents of the view (containing sprites which were rendered with a 1 to 1 pixel mapping) were slightly blurry. But views of a different size would be fine and render as intended.
Debugging through SFML I found the view actually ended up 1 pixel smaller with certain view sizes, causing the content of size X to be rendered in a view the size of X-1, thus causing the sprites to misalign.

If we look at

return IntRect(static_cast<int>(0.5f + width  * viewport.left),
                   static_cast<int>(0.5f + height * viewport.top),
                   static_cast<int>(width  * viewport.width),
                   static_cast<int>(height * viewport.height));

We can see it basically does the inverse of above, except for the last 2 values it didn't round the values. Thus due to float imprecision, it sometimes ended up 1 pixel smaller than intended.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs May 16, 2014

Member

I'm not sure if that's what SFML should force. Isn't this, like @MarioLiebisch said, a user problem? It might look like an issue when doing pixel-perfect rendering, but in other cases that might be even wanted.

Member

TankOs commented May 16, 2014

I'm not sure if that's what SFML should force. Isn't this, like @MarioLiebisch said, a user problem? It might look like an issue when doing pixel-perfect rendering, but in other cases that might be even wanted.

@Vhab

This comment has been minimized.

Show comment
Hide comment
@Vhab

Vhab May 16, 2014

Contributor

Okay, maybe I should clarify this. Viewports are always "pixel perfect". As in, they're always on full pixels.
The reason they're cast back to integers is because opengl expects these values to always be from and to a full pixel values. Viewports are never rendered on subpixels.

The example I gave above is in my experience the most common use-case for viewports. With this bug active I have to do workarounds such as add EPSILON to my width and height.
This also illustrations how this is a precision issue, as a value as small as EPSILON already shifts the value sufficient to get the desired result.

For example, without the proposed fix, I can't reliably create a viewport at pixel 10, 20 with a size of 50, 60 without using a workaround such as adding EPSILON to width and height. On some screen sizes the result would be right, on others it would be wrong. However if I used glViewport() directly (which this API ultimately wraps), this would be trivial.

The inconsistency between how top/left are treated from width/height meant for me I had to dive into SFML's code to figure out with 2 of the 4 values worked as expected, but the other 2 did not. This is another reason why I consider this a bug.

Another example would be, based around this inconsistency between left/top and width/height. This bug would permit setting up a split screen where a 1-pixel gap would be formed between 2 views, Despite the views using consistent values (where view2Top = view1Top + view1Height).
As view2Top would get rounded to the nearest value, but view1Height's value would be floored.

I hope this helps in understanding my reasoning behind the proposed change.

Contributor

Vhab commented May 16, 2014

Okay, maybe I should clarify this. Viewports are always "pixel perfect". As in, they're always on full pixels.
The reason they're cast back to integers is because opengl expects these values to always be from and to a full pixel values. Viewports are never rendered on subpixels.

The example I gave above is in my experience the most common use-case for viewports. With this bug active I have to do workarounds such as add EPSILON to my width and height.
This also illustrations how this is a precision issue, as a value as small as EPSILON already shifts the value sufficient to get the desired result.

For example, without the proposed fix, I can't reliably create a viewport at pixel 10, 20 with a size of 50, 60 without using a workaround such as adding EPSILON to width and height. On some screen sizes the result would be right, on others it would be wrong. However if I used glViewport() directly (which this API ultimately wraps), this would be trivial.

The inconsistency between how top/left are treated from width/height meant for me I had to dive into SFML's code to figure out with 2 of the 4 values worked as expected, but the other 2 did not. This is another reason why I consider this a bug.

Another example would be, based around this inconsistency between left/top and width/height. This bug would permit setting up a split screen where a 1-pixel gap would be formed between 2 views, Despite the views using consistent values (where view2Top = view1Top + view1Height).
As view2Top would get rounded to the nearest value, but view1Height's value would be floored.

I hope this helps in understanding my reasoning behind the proposed change.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila May 16, 2014

Member

I totally understand your point, and it makes perfect sense.

I vote for merging this PR.

Member

LaurentGomila commented May 16, 2014

I totally understand your point, and it makes perfect sense.

I vote for merging this PR.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs May 16, 2014

Member

Why's the viewport a FloatRect then anyway? A quick grep showed that it's converted to an IntRect everywhere, and regarding @Vhab's reasoning (which also makes sense to me), it can be changed to an IntRect. Or am I missing use cases?

Member

TankOs commented May 16, 2014

Why's the viewport a FloatRect then anyway? A quick grep showed that it's converted to an IntRect everywhere, and regarding @Vhab's reasoning (which also makes sense to me), it can be changed to an IntRect. Or am I missing use cases?

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 May 16, 2014

Member

I too think this is a bug in the current implementation.

According to documentation, glViewport is nothing more than an affine transformation from NDC to window coordinates meaning (-1,-1) gets mapped to top-left of the viewport and (1,1) gets mapped to bottom right of the viewport (because SFML flips y). The documentation also says that x and y should be in pixel coordinates so for me adding the width and height to those values should produce pixel coordinates as well.

The problem arises when we try to be smart and round ourselves. It is guaranteed that rounding behaviour will be consistent throughout the execution of the program, unless... we go ahead and do things like + 0.5f ourselves. In that case, we need to make sure that the rounding behaviour of all related values is consistent with what OpenGL expects, and in this case it isn't. Depending on the size of the window, the viewport size will be rounded differently, which produces inconsistent results that depend solely on the window size. In some cases it will be rounded up and in others not. This is the definition of a bug for me.

OpenGL already went down the right path and made the user specify viewports in integer pixel values. Why doesn't SFML enforce the same? Surely it isn't too hard for someone to compute the dimensions of their viewport on their own and pass that to SFML instead? It would put the burden of choosing how to round on the developer, but as already shown, maybe that is a better idea in the end.

Member

binary1248 commented May 16, 2014

I too think this is a bug in the current implementation.

According to documentation, glViewport is nothing more than an affine transformation from NDC to window coordinates meaning (-1,-1) gets mapped to top-left of the viewport and (1,1) gets mapped to bottom right of the viewport (because SFML flips y). The documentation also says that x and y should be in pixel coordinates so for me adding the width and height to those values should produce pixel coordinates as well.

The problem arises when we try to be smart and round ourselves. It is guaranteed that rounding behaviour will be consistent throughout the execution of the program, unless... we go ahead and do things like + 0.5f ourselves. In that case, we need to make sure that the rounding behaviour of all related values is consistent with what OpenGL expects, and in this case it isn't. Depending on the size of the window, the viewport size will be rounded differently, which produces inconsistent results that depend solely on the window size. In some cases it will be rounded up and in others not. This is the definition of a bug for me.

OpenGL already went down the right path and made the user specify viewports in integer pixel values. Why doesn't SFML enforce the same? Surely it isn't too hard for someone to compute the dimensions of their viewport on their own and pass that to SFML instead? It would put the burden of choosing how to round on the developer, but as already shown, maybe that is a better idea in the end.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila May 16, 2014

Member

Why's the viewport a FloatRect then anyway?

This is the only way to avoid breaking the current behaviour when you resize the window (i.e. the viewport gets resized too). If you give a fixed number of pixels, you have to adjust the viewport whenever the window is resized, even if you don't actually care about the viewport and just want to leave it to the full size of the window. That would force to have this kind of code in every single SFML program:

//... event loop ...
{
    if (event.type == sf::Event::Resized)
    {
        sf::View view = window.getView();
        view.setViewport(sf::IntRect(0, 0, event.size.x, event.size.y));
        window.setView(view);
    }
}
Member

LaurentGomila commented May 16, 2014

Why's the viewport a FloatRect then anyway?

This is the only way to avoid breaking the current behaviour when you resize the window (i.e. the viewport gets resized too). If you give a fixed number of pixels, you have to adjust the viewport whenever the window is resized, even if you don't actually care about the viewport and just want to leave it to the full size of the window. That would force to have this kind of code in every single SFML program:

//... event loop ...
{
    if (event.type == sf::Event::Resized)
    {
        sf::View view = window.getView();
        view.setViewport(sf::IntRect(0, 0, event.size.x, event.size.y));
        window.setView(view);
    }
}
@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 May 16, 2014

Member

According to this documentation, I get the impression that you would want to handle resize events anyway for pixel perfect rendering.

For all other cases, even with an IntRect, SFML can still internally resize the viewport based on the old viewport/size ratio. If rounding screws up for some reason, the user can still do the resizing themselves since their ratios were exotic anyway. I think many people still don't seem to understand the idea that because you specify a value in the [0,1] range it implies that the viewport is automatically resized based on the window's current size, which is why you still see a lot of code that handles resize events manually. If you ask me, not many people will miss this behaviour if it gets changed.

Just think of the scenarios. If you want automatic resize when the window's size gets bigger, you are effectively subsampling (drawing less than what would fit on the screen) and stretching. If it gets smaller, you will be supersampling and shrinking the image, relying on OpenGL filtering which is probably not what people would do when they do their own supersampling. But the biggest downside of this behaviour is forcing the user to enforce that the window's aspect ratio stays the same. If they allowed arbitrary resizing, text and sprite rendering could look horrible depending on the new size of the window. No other game I've seen does this. This means they either disable resizing all together or check the new size of the window and lock it to the nearest acceptable size.

This use case that you have in mind where the user would actively want automatic viewport resizing for sub-sections of the window is less common than you might expect.

Member

binary1248 commented May 16, 2014

According to this documentation, I get the impression that you would want to handle resize events anyway for pixel perfect rendering.

For all other cases, even with an IntRect, SFML can still internally resize the viewport based on the old viewport/size ratio. If rounding screws up for some reason, the user can still do the resizing themselves since their ratios were exotic anyway. I think many people still don't seem to understand the idea that because you specify a value in the [0,1] range it implies that the viewport is automatically resized based on the window's current size, which is why you still see a lot of code that handles resize events manually. If you ask me, not many people will miss this behaviour if it gets changed.

Just think of the scenarios. If you want automatic resize when the window's size gets bigger, you are effectively subsampling (drawing less than what would fit on the screen) and stretching. If it gets smaller, you will be supersampling and shrinking the image, relying on OpenGL filtering which is probably not what people would do when they do their own supersampling. But the biggest downside of this behaviour is forcing the user to enforce that the window's aspect ratio stays the same. If they allowed arbitrary resizing, text and sprite rendering could look horrible depending on the new size of the window. No other game I've seen does this. This means they either disable resizing all together or check the new size of the window and lock it to the nearest acceptable size.

This use case that you have in mind where the user would actively want automatic viewport resizing for sub-sections of the window is less common than you might expect.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila May 16, 2014

Member

According to this documentation, I get the impression that you would want to handle resize events anyway for pixel perfect rendering.

I think most people don't care. At least not until they polish their game, which as you know never happens ;)
I'm pretty sure that removing automatic viewport resizing would lead to hundreds of posts on the forum, asking why garbage appears when you resize your SFML window.

For all other cases, even with an IntRect, SFML can still internally resize the viewport based on the old viewport/size ratio.

I'm not sure it's a good idea to internally change something that the user explicitly set. If I call setViewport, resize my window, and call getViewport, it would be cool to get back the initial viewport. Relative coordinates solve this issue, that's why I chose them.

Member

LaurentGomila commented May 16, 2014

According to this documentation, I get the impression that you would want to handle resize events anyway for pixel perfect rendering.

I think most people don't care. At least not until they polish their game, which as you know never happens ;)
I'm pretty sure that removing automatic viewport resizing would lead to hundreds of posts on the forum, asking why garbage appears when you resize your SFML window.

For all other cases, even with an IntRect, SFML can still internally resize the viewport based on the old viewport/size ratio.

I'm not sure it's a good idea to internally change something that the user explicitly set. If I call setViewport, resize my window, and call getViewport, it would be cool to get back the initial viewport. Relative coordinates solve this issue, that's why I chose them.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 May 16, 2014

Member

Hmm... I guess in that case this shouldn't be changed and this fix makes sense. Like many other things, this won't affect the way I use SFML myself, so I can't really speak for the people who this will affect 😉.

Member

binary1248 commented May 16, 2014

Hmm... I guess in that case this shouldn't be changed and this fix makes sense. Like many other things, this won't affect the way I use SFML myself, so I can't really speak for the people who this will affect 😉.

@LaurentGomila LaurentGomila added this to the 2.2 milestone May 16, 2014

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs May 19, 2014

Member

Yeah, fix makes sense in this case. :)

Member

TankOs commented May 19, 2014

Yeah, fix makes sense in this case. :)

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r May 26, 2014

Member

Is this accepted & tested? Can it be merged?

Member

eXpl0it3r commented May 26, 2014

Is this accepted & tested? Can it be merged?

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila May 27, 2014

Member

For me it's ok. Not tested, but the fix is really trivial.

Member

LaurentGomila commented May 27, 2014

For me it's ok. Not tested, but the fix is really trivial.

@TankOs TankOs self-assigned this May 27, 2014

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs May 27, 2014

Member

Merged, thank you. :)

Member

TankOs commented May 27, 2014

Merged, thank you. :)

@TankOs TankOs closed this May 27, 2014

@Vhab Vhab deleted the Vhab:bugfix/rendertarget branch May 27, 2014

@eXpl0it3r eXpl0it3r added resolved and removed confirmed labels May 28, 2014

@Vhab Vhab unassigned TankOs Apr 30, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment