Skip to content

Implementing SSBO for jME.#822

Closed
JavaSaBr wants to merge 19 commits into
jMonkeyEngine:masterfrom
JavaSaBr:added_new_var_type
Closed

Implementing SSBO for jME.#822
JavaSaBr wants to merge 19 commits into
jMonkeyEngine:masterfrom
JavaSaBr:added_new_var_type

Conversation

@JavaSaBr
Copy link
Copy Markdown
Contributor

@JavaSaBr JavaSaBr commented Feb 8, 2018

@Nehon
Copy link
Copy Markdown
Contributor

Nehon commented Feb 8, 2018

Nice.
However I have several remarks:

  1. SSBO are ogl 4.3 only feature... it make it hard to use for the engines shaders. But at least users can decide to use them
  2. I feel this has a lot in common with Uniform Buffer Objects and maybe we could also implement them.
  3. The API to use them is not good at all:
    https://github.com/JavaSaBr/jmonkeybuilder/blob/feature/multi_select_in_scene/src/test/java/com/ss/editor/test/external/SSBOSceneTest.java#L40-L54
    Updating the byteBuffer directly is horrible, here you are mapping data (a color?) to light_1 in this shader https://github.com/JavaSaBr/jmonkeybuilder/blob/feature/multi_select_in_scene/src/test/resources/MatDefs/Unshaded.frag#L15-L19 but you have to know your binary....

We should have a java object with the same structure as you expect in the shader set as a materialParam, and the SSBO implem would handle the transformation to byte buffer.
Your ShaderStorageBufferObject class should be called BufferObject. It would offer an API to set the params like set("varName", value). Behind the scene, the BufferObject would update the underlying byte buffer.

Then the Material should have a convenience method setBuffetObject (maybe with an additional param to differentiate SSBO and UBO ...)

your example would look like:

BufferObject ssbo = new BufferObject(2); //2 is the binding
ssbo.set("light_1", new ColorRGBA(0, 0, 1,1));
material.setBufferObject("TestSSBO", ssbo);

Copy link
Copy Markdown
Contributor

@Nehon Nehon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Api needs to be reworked

@JavaSaBr
Copy link
Copy Markdown
Contributor Author

JavaSaBr commented Feb 8, 2018

@Nehon ok, I will continue to work on this ;)

@Nehon
Copy link
Copy Markdown
Contributor

Nehon commented Feb 8, 2018

Also more things.
UBO and SSBO can be shared across shaders, so maybe it has no sense to set it as a material param, because as soon as you set it on a material it's available in all shader programs so in all materials.
Maybe it should be set on the Renderer directly and bond as a g_ global parameter.

Also UBO and SSBO have a size limit in the spec. 16kb for UBO an 128mb for SSBO, though implems might provide more. There must be a way to query this value, this should be added to the limit enum of the renderer when UBO and SSBO are supported.
Also the BufferObject implementation should check that the byte buffer data doesn't exceed this limit and throw and appropriate error if ever.

Maybe you should open this as a thread on the forum so that other devs can chime in and give their opinion on the implementation.

@JavaSaBr
Copy link
Copy Markdown
Contributor Author

JavaSaBr commented Feb 8, 2018

@Nehon
Copy link
Copy Markdown
Contributor

Nehon commented Feb 26, 2018

Rha... Sorry, the merge of your other PR introduced conflicts on this one.
Could you rebase and fix the issues please?

@JavaSaBr
Copy link
Copy Markdown
Contributor Author

@Nehon I will update it later.

# Conflicts:
#	jme3-core/src/main/java/com/jme3/renderer/opengl/GL.java
#	jme3-core/src/main/java/com/jme3/renderer/opengl/GL3.java
#	jme3-core/src/main/java/com/jme3/renderer/opengl/GL4.java
#	jme3-lwjgl3/src/main/java/com/jme3/renderer/lwjgl/LwjglGL.java
@JavaSaBr
Copy link
Copy Markdown
Contributor Author

JavaSaBr commented Mar 4, 2018

@Nehon I have resolved conflicts.

@Nehon
Copy link
Copy Markdown
Contributor

Nehon commented May 18, 2018

I rebased the PR to be able to merge it... Somehow I failed to repush to this PR so I created another #863
It's now been merged in master. Thanks for your work!

@Nehon Nehon closed this May 18, 2018
@shadowislord
Copy link
Copy Markdown
Member

  • There are no tests for this feature (unit tests or examples). We don't actually know if the feature works or not
  • It's not explained what the value is of either UBO or SSBO to the engine and/or users
  • It's not a good idea to implement a feature that's not used by the engine itself because it will suffer from bit rot and stop working with time
  • One of the main benefits of UBOs is the ability to pass a variety of data types in a single buffer object, which is more efficient. The engine could have taken advantage of this by, say, passing material parameters and global parameters in two separate UBOs. With the current implementation the user has to be using UBOs explicitly which is not convenient and a leaky abstraction

@Nehon
Copy link
Copy Markdown
Contributor

Nehon commented May 22, 2018

https://hub.jmonkeyengine.org/t/buffer-objects-in-jme/40054

There has been a long discussion here. Also the Pr stayed open several months... It's somehow too bad your objections comes 2 days after the Pr has been merged.

Though nothing is written in stone. We can change how it's done. What do you suggest ? Maybe chime in on the forum thread.

@shadowislord
Copy link
Copy Markdown
Member

I'm not objecting to the PR per se, I'm just saying that its a good idea to approach this from a different angle. Instead of saying, "Hey let's implement UBOs and SSBOs", you should instead say, "How can we make the engine faster with UBOs / SSBOs?" or "What kind of features can we implement if we had UBOs / SSBOs that otherwise we couldn't have?"

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.

3 participants