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

Fix waitFor not being implemented in lwjgl3 #1048

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@tlf30
Copy link
Contributor

commented Mar 19, 2019

So this seems like a big bug to me. I noticed that for some reason for the lwjgl 3 display context, the wait for was being ignored. There was a comment about a OSX fix, but with no information to go with it.

It seems to me that it is important for the waitFor to work, as it broke one of my applications that I was swapping from lwjgl2 to lwjgl3.

Does anyone know about this, or can provide insight into this?
This PR adds support for the waitFor in the same way that it was implemented in lwjgl2.
Also, if someone can confirm that this still works for them, that would be great.

PS: I hope I opened this PR correctly. If not I'm sorry in advance, every project has their preferred way of doing things.

Thank you,
Trevor Flynn

@tlf30

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

Not sure why the compare is not working correctly, only a couple lines changed...

@pspeed42

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

Probably you checked the file in with a different line ending. You may want to fix it as it's harder to see what actually changed.

@tlf30

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

Ah, I did not think about that. I'll fix it right now

@MeFisto94

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

FYI: https://github.blog/2018-05-01-ignore-white-space-in-code-review/
As pointed out below:
https://github.com/jMonkeyEngine/jmonkeyengine/pull/1048/files?w=1 also works

Note though that this still means we don't want "unclean" commits, as git tools might not ignore whitespaces and it's not clear which line ending/white space is the correct one.

Actually we should have linters/auto formatters as bots which comment on the PRs :)

@pspeed42

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

Or I sort of remember that it's possible to set line ending style on the repo or something... but that may make a whole bunch of source change because I don't believe it's consistent right now.

}

// NOTE: this is required for Mac OS X!
mainThread = Thread.currentThread();

This comment has been minimized.

Copy link
@MeFisto94

MeFisto94 Mar 19, 2019

Member

I know what they are doing here and your method might break OSX support. Do some research on the hub please:
For OpenGL to work on Mac OS X it has to be started on the MainThread (which is the Thread the Application is started with, hence Thread.currentThread(). Your change destroys this. So it changes semantics.

WaitFor() might be useless because if you're already on the MainThread, you don't have to wait for it.
But it's an inconsistent behavior as lwjgl2 probably returns from the Application#start() call.
So a proper solution would be to document this or provide AppSettings to enable "forking", unless on OS X.
Or always use the CurrentThread, which to me is expectable if you hand control over to a framework.

This comment has been minimized.

Copy link
@tlf30

tlf30 Mar 19, 2019

Author Contributor

Would it be better to check if we are on OS X, then ignore the wiatFor. Also, does this mean that jme-lwjgl does not work on OS X?

@tlf30 tlf30 force-pushed the tlf30:waif-for-fix-lwjgl3 branch from 02cb54d to 4c86de0 Mar 19, 2019

@tlf30

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

OK I have line endings fixed.

@tlf30

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

I would also like to note, that with LWJGL3, the headless context does return from start, which makes inconsistent behavior within the library itself.

@tlf30

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

OK, I just made a new commit. If waitFor is false, we run it on the main thread, otherwise, create a new thread. How about this solution?

@tlf30

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

I do not have an apple computer, perhaps someone can test this?

tlf30 added some commits Mar 19, 2019

@tlf30

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

Just for documentation purpose, it was issue #433 that has the os x workaround info in it.

I have tested this proposed fix on my apps in windows, and I think it is ready but still needs tested in OS X. Any thoughts?

Thanks,
Trevor

@stephengold

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

Perhaps someone at the Forum would be willing to help us.

@Jeddic

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

What exactly needs to be tested here? Is there some sort of test case to run?

@tlf30

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

Just needs to be tested in a working application. Behavior should not change on OS X and that needs to be confirmed.

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.