-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Basic support for Transform Feedback #1078
base: master
Are you sure you want to change the base?
Conversation
@@ -34,6 +34,8 @@ | |||
import com.jme3.export.*; | |||
import com.jme3.scene.Mesh; | |||
import java.io.IOException; | |||
import java.util.Arrays; | |||
import java.util.Objects; |
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.
Unused imports
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.
thank you for pointing them out, i'll fix these with next commit
public static boolean supports(Collection<Caps> caps, QueryObject query) { | ||
switch(query.getType()) { | ||
case SamplesPassed: | ||
//GL1.5 is available or (GL_ARB_occlusion_query) |
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.
So.. are we supposed to test if 1.5 is available?
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.
I am under the impression JME is GL 2.0+ so no need to test?
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.
What about GL ES?
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.
This will have to be investigated, a quick check I made, it seems gl es 2.0 might not support occlusion queries.
import com.jme3.scene.VertexBuffer; | ||
import com.jme3.shader.BufferObject; | ||
import com.jme3.shader.UniformBufferObject; |
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.
Unused import
/** | ||
* Started queries. | ||
*/ | ||
public int[] startedQueries = new int[6/*QueryObject.Type.values().length*/]; |
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.
What is the point of this? Just to remember which queries where started? Do we really need it?
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.
Yes, unless a query is started issuing a glEndQuery or checking for result yield a glerror. This is here to prevent that. Furthermore, take a look at the following
void glBeginQuery(GLenum target, GLuint id);
void glEndQuery( GLenum target);
Do you see something off about the functions above?
The off thing about them is that glEndQuery does not take query id as a parameter!
Thus in other words lets say I make two timer queries:
queryT1 = new QueryObject(app.getRenderer(), QueryObject.Type.TimeElapsed);
queryT2 = new QueryObject(app.getRenderer(), QueryObject.Type.TimeElapsed);
If you run the following code:
queryT1.begin();
queryT2.begin();
//some work
queryT2.end();
queryT1.end();
You will correctly get a RenderException (at line queryT2.begin();
)
com.jme3.renderer.RendererException: Query of type TimeElapsed already started.
Without the render exception you would get gl error. In other words you are not allowed to nest the same type of query.
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.
Sure, but this is never exposed outside of the core. So maybe we don't need those checks ...
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.
Are you saying that users will not be able to use QueryObject? I believe we do need them as otherwise its pretty easy to get gl errors.
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.
As usual there is nothing stopping you from using internals in jme, but imo this is not an end user feature so maybe we can avoid having all those extra checks.
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.
Why not have the checks? Those checks are not expensive. You are not starting 10 thousands of queries per frame. They only bring robustness. So I do not see a reason for them not to be there.
Also even if it was used just in internals, there's always a risk that you can nest some queries and this would be a nightmare for figuring out such error.
Eg. Imagine you use TimerQuery while you also have DetailedProfiler attached. And there you go... nested queries.
package com.jme3.renderer; | ||
|
||
import com.jme3.util.NativeObject; | ||
import java.util.EnumMap; |
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.
Unused import?
*/ | ||
CanEnd | ||
} | ||
|
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.
Is this really needed?
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.
Refer to the answer above, in short, to prevent gl errors.
Adds basic support for Transform Feedback (TF).
As part of discussion on discord
To support basic transform feedback.
The use of this method is similar to how rendering of framebuffers is handled in jme. This method is called to enable transform feedback, after which rendered geometry's TF ouput will be captured to the specified output buffers. Calling it will null, disables transform feedback.
In order to use TF, a shader has to specify the output variables which will be outputted to TF. This is accomplished by specifying the output variables in Technique block of .j3md file. Syntax is:
For example a vertex/geometry shader contains the ouput:
A technique can be written as:
Awaiting your feedback.