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

bgfx examples #6

Merged
merged 4 commits into from
Oct 1, 2016
Merged

bgfx examples #6

merged 4 commits into from
Oct 1, 2016

Conversation

code-disaster
Copy link
Contributor

Java ports of bgfx examples to accompany LWJGL/lwjgl3#240.

bgfx examples migrated from lwjgl-bgfx development branch.
@httpdigest
Copy link
Member

httpdigest commented Sep 25, 2016

Let's merge this once org.lwjgl:lwjgl-bgfx is in oss.sonatype.org.

@Spasi
Copy link
Member

Spasi commented Sep 29, 2016

The latest nightly build includes the bgfx bindings. There have been some breaking changes since this PR, e.g. bgfx_set_platform_data is now in the BGFXPlatform class.

@httpdigest
Copy link
Member

httpdigest commented Sep 29, 2016

Now, the bgfx bindings do some invalid parameter validations. The following usecase is now not possible anymore: https://github.com/bkaradzic/bgfx/blob/master/examples/02-metaballs/metaballs.cpp#L608-L610
because bgfx_alloc_transient_vertex_buffer checks the tvb via BGFXTransientVertexBuffer.validate(_tvb) which in turn checks whether DATA has been set, which it has not, because that field will be filled by the bgfx call.

Exception in thread "main" java.lang.NullPointerException
    at org.lwjgl.system.Checks.checkPointer(Checks.java:102)
    at org.lwjgl.bgfx.BGFXTransientVertexBuffer.validate(BGFXTransientVertexBuffer.java:315)
    at org.lwjgl.bgfx.BGFX.nbgfx_alloc_transient_vertex_buffer(BGFX.java:2113)
    at org.lwjgl.bgfx.BGFX.bgfx_alloc_transient_vertex_buffer(BGFX.java:2125)
    at org.lwjgl.demo.bgfx.Metaballs.frame(Metaballs.java:531)
    at org.lwjgl.demo.bgfx.Demo.run(Demo.java:160)
    at org.lwjgl.demo.bgfx.Metaballs.main(Metaballs.java:359)

@code-disaster
Copy link
Contributor Author

Yes, this applies to bgfx_transient_index_buffer_t.data and bgfx_transient_vertex_buffer_t.data.

@Spasi
Copy link
Member

Spasi commented Sep 29, 2016

because that field will be filled by the bgfx call

Thanks, will be fixed in the next nightly with LWJGL/lwjgl3@7808282.

@httpdigest
Copy link
Member

httpdigest commented Sep 30, 2016

Is there already a consensus on how LWJGL 3 is going to handle the bgfx shared library? Currently, when you just checkout the lwjgl3-demos repository and download all the Maven dependencies, you cannot run bgfx demos without manually building bgfx as a shared library (or downloading it from somewhere else).
I think we should handle the bgfx shared library in the same way that we handle glfw, by building it on a CI server (travis, appveyor) and include it in a natives jar.

@Spasi
Copy link
Member

Spasi commented Sep 30, 2016

I initially thought that bgfx is statically configured for specific backends, but I was wrong. The default build is small enough and supports all backends on a given platform. So yes, I'll setup CI builds.

The builds will be configured with --with-glfw --with-shared-lib (and --with-ovr on Windows). As always, users will be able to replace the shared library with a custom build (debug, disabled backends for smaller binaries, etc).

@code-disaster
Copy link
Contributor Author

code-disaster commented Sep 30, 2016

You don't need --with-glfw. I didn't manage to compile it when I tried, and I didn't pursue it any further when I found it's not required. The integration with lwjgl-glfw is simple enough.

--with-ovr has the "disadvantage" that, for Oculus owners like me, the damn Oculus Store pops up each time I start a program which links to their SDK, no matter if I want to even use it. I'm not sure if that's a bgfx thing or just the way this works. I could unplug my Rift each time I'm coding, but for now I just opted to leave it be and compile a bgfx.dll without this flag.

In any case, it's nice that LWJGL provides the means to replace the shared lib at any time, if needed.

@httpdigest
Copy link
Member

I also did not compile with --with-glfw, and it seems to work (on Windows).

@Spasi
Copy link
Member

Spasi commented Sep 30, 2016

You don't need --with-glfw.

I'm afraid it's going to break badly under MacOS without --with-glfw (which is just an alias for BGFX_CONFIG_MULTITHREADED=0). Will try and let you know.

--with-ovr has the "disadvantage" that, for Oculus owners like me, the damn Oculus Store pops up each time I start a program which links to their SDK, no matter if I want to even use it.

OK, will build without it, until that issue is fixed and/or the Oculus user base increases.

@httpdigest
Copy link
Member

I propose to add this code in Demo.java right below the GLFW window creation to fix window resizing and aspect ratio issues in all demos:

GLFWFramebufferSizeCallback fbsc;
glfwSetFramebufferSizeCallback(window, fbsc = new GLFWFramebufferSizeCallback() {
    public void invoke(long window, int width, int height) {
        Demo.this.width = width;
        Demo.this.height = height;
        bgfx_reset(width, height, Demo.this.reset);
    }
});

@LWJGL-CI
Copy link

LWJGL-CI commented Oct 1, 2016

The latest nightly build (28) includes bgfx shared libraries.

@httpdigest
Copy link
Member

httpdigest commented Oct 1, 2016

There does not seem to be bgfx natives on Maven/oss.sonatype.org.

@Spasi
Copy link
Member

Spasi commented Oct 1, 2016

Sorry about that, should be OK now.

@httpdigest
Copy link
Member

Yes, seems to work fine. @code-disaster, could you add the necessary changes to the pom.xml in the profiles section to include the natives, please? Then we can merge the pull request.

@httpdigest httpdigest merged commit 547f42c into LWJGL:master Oct 1, 2016
@httpdigest
Copy link
Member

httpdigest commented Oct 1, 2016

Thanks for your work of porting the bgfx demos!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants