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
Workaround for macOS OpenGL performance issues. #5893
base: master
Are you sure you want to change the base?
Conversation
e68702f
to
52d58ae
Compare
52d58ae
to
3d9670d
Compare
| @@ -66,8 +70,13 @@ public DefaultTextureBinder (final int method, final int offset, final int count | |||
| } | |||
|
|
|||
| public DefaultTextureBinder (final int method, final int offset, int count, final int reuseWeight) { | |||
| final int max = Math.min(getMaxTextureUnits(), MAX_GLES_UNITS); | |||
| if (count < 0) count = max - offset; | |||
| int max = Math.min(getMaxTextureUnits(), MAX_GLES_UNITS); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should let final no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but it's probably not worth the effort to change back.
| if (count < 0) count = max - offset; | ||
| int max = Math.min(getMaxTextureUnits(), MAX_GLES_UNITS); | ||
| if (count < 0) { | ||
| if (enableMacOSWorkaround && UIUtils.isMac && Gdx.app.getType() == ApplicationType.Desktop) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no other method to know if the application run on macOs? It include a class from scene2d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not as far as I could see. At least none compatible with gwt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PokeMMO
UIUutils#isMac does not check if it's mac os and is not compatible with GWT, why?
I think it does, also in GWT.
Lines 9 to 12 in 8f09a42
| public class UIUtils { | |
| static public boolean isMac = Navigator.getPlatform().contains("Mac"); | |
| static public boolean isWindows = Navigator.getPlatform().contains("Win"); | |
| static public boolean isLinux = Navigator.getPlatform().contains("Linux"); |
So if the check isMac works, i'd not recommend to add
DefaultTextureBinder#enableMacOSWorkaround
btw: #5910 extends UIUtils by UIUtils#isAndroid and UIUtils#isIos, the pr is currently in progress to test, may if you request changes to this constants, i could look for a solution that ensures that it's mac, if it's not doing right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UIUutils#isMac works fine, we also check if it's a desktop app to avoid interfering with GWT.
DefaultTextureBinder#enableMacOSWorkaround exists to allow users to disable this workaround in their own apps more easily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DefaultTextureBinder#enableMacOSWorkaround exists to allow users to disable this workaround in their own apps more easily.
OK, so my 2nd question: If we have a DefaultTextureBinder#enableMacOSWorkaround flag that is only used for some few or only(?) an architecture of mac, why it does not work for all? And does the developer have to identify when to enable this? And how to handle this while deploying, shouldn't do this libgdx automatically, depending on the cpu / system architecture you are using / the app is deployed?
and my third question is:
https://stackoverflow.com/a/42320505 is the answer of your first link, and in the issue on stack, this guy used more than MAX_GLES_UNITS as count(?), so isn't the solution to reduce the count to the correct value instead of to 1?
or did i get something wrong right now?
They are talking about
glBindAttribLocation, glGetAttribLocation, glActiveTexture(GL_TEXTURE0) and glActiveTexture(GL_TEXTURE0 + activeTexture);
|
This would just disable any mutli-texturing, no? Also the first and last link (the second being undetermined) seem to be talking about two different things. They might be related, but that would be need to be confirmed. |
Effectively yeah.
Looks like it yeah, I was linked to it after I made the PR and it looked like the same issue, but seems it's only similar. The underlying cause might be the same, but you're right we should verify this. |
|
not sure it disable multi-texturing, i think it forces to use texture unit 1 (not zero), see ModelBatch default texture binder offset. I agree, it needs more tests to confirm what's the issue exactly. I'd try several things: ROUND_ROBIN method, force to texture unit 0, other units, force count to 2, more. Also, take a look at GLProfiler stats : how many shader switches you have, what behavior with less switches. I don't have Mac hardware so i can't help. If issue is confirmed, i would add a new binding strategy (method) "DISABLED" instead of forcing count/offset. Also add some documentation to warn users that parameters could not be used. It's a breaking change if user code rely on texture slots. |
|
Well then it might work for 2 textures, but for any more it won't. Were you experiencing this issue yourself @PokeMMO ? |
Not personally, but another member of my team has recreated it. (Along with a slow, but steady stream of player complaints) Setting it to round-robin, offset to 0, count to 2 still has the issue. The only time we've gotten decent results seems to be with count 1. It seems to me the issue is more in the changing of the texture index instead of what it actually is. So offset 1 vs offset 0 has a minimal difference. |
|
I've deployed this workaround and have confirmation that this resolves the issue from several users. |
|
This PR can't be merged as is because it disable multi texturing. In order to prevent texture unit changes, a clean solution would be :
However, since it seams related to some weird macos driver, i'm not sure it worth to make that change since it could cause more texture switches with good mac drivers.
So, we don't have enough information to choose at runtime between Minimize texture switches VS minimize shader unit changes. |
|
servo/pathfinder#337 seems to be resolving the same/a similar issue on another project. Their solution seems like it might be a better one. |
Some macOS users experience a large performance degradation rendering 3D with default ModelBatch settings. This workaround seems to greatly alleviate this issue.
This issue may be specific to AMD graphics drivers, and if so we may want to limit this workaround to AMD GL renderer strings.Update: Third link says it also afflicts nvidiaIf there are any better ways to handle this, I'm all ears.
UIUtils.isMac used because it already handles gwt emulation here.
See: