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

Windows DesktopGL everything is blurry. Half pixel offset missing for BasicEffect? #6619

Open
roy-t opened this issue Jan 3, 2019 · 20 comments

Comments

Projects
None yet
6 participants
@roy-t
Copy link
Contributor

commented Jan 3, 2019

What operating system are you using:

  • Windows, DesktopGL

There is a rendering difference between the DirectX and OpenGL versions of MonoGame when using the BasicEffect. Because the half pixel offset for OpenGL is not taken into account. While this is taken into account for the SpriteBatch:
https://github.com/MonoGame/MonoGame/blob/faea8a6a89504673e2bb7e435f0da8cc513d8c30/Mono
Game.Framework/Graphics/GraphicsDevice.OpenGL.cs#L272

This means that anything rendered with the BasicEffect shader is slightly blurry on OpenGL. For example see these two screenshots:

OpenGL:
text_a

DirectX:
text_c

I think we should give the user some way to know that the half pixel offset should be set by themselves or change the OpenGL shader for the BasicEffect to fix this.

What do you think? I can make a PR once I've gathered more input :).

@mrhelmut

This comment has been minimized.

Copy link
Contributor

commented Jan 3, 2019

We could use preprocessing directives to put OpenGL specific stuff in .fx files with #ifdef OPENGL and rebuild BasicEffect.ogl.mgfxo?

(ImGUI FTW)

@tomspilman

This comment has been minimized.

Copy link
Member

commented Jan 3, 2019

This means that anything rendered with the BasicEffect shader is slightly blurry on OpenGL

We could likely fix this in BasicEffect.

But i don't know if we can fix this in a generic way for all custom effects. If we could then that solution would work for everything including SpriteBatch and other stock effects.

@Jjagg - I guess this might already be something handled by the HLSL to GLSL cross compiling tech we're looking at using?

@tomspilman tomspilman added this to the 3.8 Release milestone Jan 3, 2019

@Jjagg

This comment has been minimized.

Copy link
Contributor

commented Jan 3, 2019

Hold up, there is already a half pixel offset being applied for OpenGL and not for DX. The current behavior is completely wrong, but there's an explanation for how it got like this, from my understanding:

DX9 is the weird one out. All other graphics APIs do it 'right'. XNA didn't because it used DX9. Though it applied a half pixel offset for SpriteBatch. So MG technically needs the half-pixel offset on all platforms that do not use DX9 if we want to be XNA compatible. The DX backend doesn't have the offset applied I think because it evolved from DX9 and it was forgotten, though it should to be XNA compatible (looking back this is part of why #5144 failed). XNA applies half a pixel offset in SpriteBatch so DX behavior was consistent there which we prove in testing. For OpenGL the half pixel offset is applied to every effect that goes through MojoShader. Somewhere along the line someone must've thought that OpenGL spritebatch rendering wasn't consistent with DX, due to that half pixel offset for every effect, so a reverse half pixel offset to spritebatch was added.

So bottom line is, we need to get rid of the half pixel offset in SpriteBatch for OpenGL. We need to decide whether we want to force the half pixel offset for all our platforms to better be compatible with XNA or if we want to move forward and get rid of the hack and fix this issue once and for all.

@tomspilman Not sure if any consoles use DX9 API, maybe Vita?

For reference:

  • SpriteBatch 'reverse' half pixel offset 'fix' (NeedsHalfPixelOffset is set to true for OpenGL)
  • pixel offset for GL 'fix'
  • lines where the offset variable is added in MojoShader
@Jjagg

This comment has been minimized.

Copy link
Contributor

commented Jan 3, 2019

@roy-t If we do move away from XNA behavior and make things consistent with our DX implementation, the right fix would be to remove the lines here and leave posFixup[2] and posFixup[3] at 0:

_posFixup[2] = (63.0f/64.0f)/Viewport.Width;
_posFixup[3] = -(63.0f/64.0f)/Viewport.Height;

If we go the other way around, we need to apply the half pixel offset in SpriteBatch on all platforms and somehow fix the vertex position in all shaders. That sounds a lot more painful to me, and considering the non-SpriteBatch parts of MG's DX implementation have been inconsistent with XNA for so long (and it's kind of our reference implementation) I'm strongly in favor of dropping the offset altogether and XNA compatibility in this regard with it.

@roy-t

This comment has been minimized.

Copy link
Contributor Author

commented Jan 3, 2019

Thanks for the detailed information. It is a lot more complex than I first thought then. I just thought it was forgotten for the basic effect. I'll hold off on changing anything until there is a consensus :). But after that the fix should be really small it seems.

@willmotil

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2019

I say fix it before everyone forgets about it and then it becomes a mess later.

If the concern to maintain compatibility is great then turn it off as well but make it a option that can be turned on.

Add a Dx9LegacyPixelOffsetCompatability bool Property to Game or graphics or some were that the user can just turn on just like we do with IsMouseVisible = true; and the like.

Wrap each of those code parts in a if( Game.Dx9LegacyPixelOffsetCompatability ){ /* do the pixel offset */ } or some similar scheme.

Then you get to frame your cake and eat it too.

I think this can be bandaided though by passing in your own basic effect and adding a half pixel translation to the projection matrix.

@roy-t

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2019

@Jjagg after thinking about it a bit more, I'm a bit confused. So if I target Windows+DirectX do I need to add the half pixel offset to my HLSL shaders, just like in the old XNA/DirectX 9 days? And then if the HLSL shader is auto-translated to GLSL it will add an additional ReverseOffset? Or can I forget about the half pixel offset for DirectX? In that case it seems the behavior is already incompatible with XNA, so it might be OK to break it completely there so we can remove the half pixel entirely?

@Jjagg

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2019

@roy-t My post was maybe a bit verbose and hard to parse.

The MG OpenGL implementation is consistent with XNA so it'd need half a pixel offset and the MG DX implementation is not consistent with XNA (except for SpriteBatch) so it does not. ImGui doesn't expect a DX9 renderer, so things look blurry when rendered with a DX9 compatible renderer like MG's GL backend.

In that case it seems the behavior is already incompatible with XNA, so it might be OK to break it completely there so we can remove the half pixel entirely?

Yes, that's exactly my point as well. MG DX has been incompatible with XNA ever since it's using DX11, so let's make everything work that way.

@mrhelmut

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2019

Wait, MojoShader adds a pixel offset to vertices, and then MG adds another (doing twice the job), and then the SpriteBatch removes one? Pfeww, funny how it ended up being like this...

@roy-t If we do move away from XNA behavior and make things consistent with our DX implementation, the right fix would be to remove the lines here and leave posFixup[2] and posFixup[3] at 0.

I've tested that and removed the NeedsHalfPixelOffset from OpenGL and everything looks fine to me. SpriteBatch rendering is 1:1 compared to what it is now.

Consoles don't use half pixel and all have DX11/OpenGL style API (so I would expect them to not be XNA accurate as well).

@Jjagg

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2019

Wait, MojoShader adds a pixel offset to vertices, and then MG adds another (doing twice the job), and then the SpriteBatch removes one? Pfeww, funny how it ended up being like this...

Not exactly. MojoShader adds the parameter for offset, MG sets it for all effects and then undoes it in SpriteBatch. That's technically correct behavior to be compatible with XNA.

@tomspilman Can you weigh in? I'd like to remove the half pixel offset and be done with this mess. If we remove it altogether we'll get consistent behavior on all platforms, SpriteBatch will render like in XNA and other renders will be offset half a pixel compared to XNA (as it is currently in MG DX).

@mrhelmut

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2019

MojoShader only exposes the parameter, alright!

Do we agree that both posFixup and NeedsHalfPixelOffset need to be removed to have consistent rendering between MG DX and MG GL?

@Jjagg

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2019

Do we agree that both posFixup and NeedsHalfPixelOffset need to be removed to have consistent rendering between MG DX and MG GL?

Yes, that's a fact. Just waiting for Tom to confirm that breaking XNA compatibility is better than hacking every platform to enforce the half pixel offset.

@tomspilman

This comment has been minimized.

Copy link
Member

commented Jan 4, 2019

So don't have time to read everything up above here.

IMO keeping XNA compatibility is critical. We should do that wherever we can.

However we have to move forward and fix XNA's sins too.

Ideally there would be some UseStandardPixelAddressing type of global we can use to "opt-in" to breaking XNA compatibility. We would document it as being something one can disable. Maybe even including it in all the template projects with a nice descriptive comment.

@Jjagg

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2019

I'm gonna implement the fix for this. We'll add a UseStandardPixelAddressing static boolean property to GraphicsDeviceManager that by default is set to false. When set to false we'll behave like XNA by applying the half-pixel offset, so the default behavior is compatible with XNA. For new projects we'll recommend setting this flag to true with documentation and by setting the flag to true in the project templates.

Leaving the flag to false will render things as they are currently rendered by MG on OpenGL platforms. Setting it to true will render things as they are currently rendered by MG on DX platforms. E.g. for ImGui.NET the flag should be set to true.

Note that this will change the behavior for DX projects after upgrading. For SpriteBatch nothing will change, if UseStandardPixelAddressing is set we'll use a reverse offset in SpriteBatch (as is currently done for OpenGL).

@RUSshy

This comment has been minimized.

Copy link

commented Jan 4, 2019

hacks on hack on hack on hacks, time to ditch xna no ?

@mrhelmut

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2019

hacks on hack on hack on hacks, time to ditch xna no ?

The plan is to keep a branch XNA accurate (there are still a bunch of relevant uses), and to move away from it in another one.

@tomspilman

This comment has been minimized.

Copy link
Member

commented Jan 4, 2019

@Jjagg - UseStandardPixelAddressing was just a off hand guess at a name. Maybe put a little more thought into it than i did. :)

@Jjagg

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2019

Alternatively we could go with something less technical that expresses the intent, like Dx9Compatible or XnaCompatible. I like UseStandardPixelAddressing better than HalfPixelOffset or something like that, because it implies that non-XNA compatible is the standard currently.

@roy-t

This comment has been minimized.

Copy link
Contributor Author

commented Jan 5, 2019

Awesome, sorry for stirring up the beehive, thanks for picking everything up. Since Jjagg is working on this, I'll try and see if there is another (small) issue that I can dedicate a bit of time to in the coming weeks. Thanks so much!

@Jjagg

This comment has been minimized.

Copy link
Contributor

commented Jan 5, 2019

Awesome, sorry for stirring up the beehive

Thanks for reporting, this was long overdue so a poke was needed :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.