-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Replaces fullscreen quad with a viewport covering triangle #2589
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
base: master
Are you sure you want to change the base?
Conversation
|
🖼️ Screenshot tests have failed. The purpose of these tests is to ensure that changes introduced in this PR don't break visual features. They are visual unit tests. 📄 Where to find the report:
✅ If you did mean to change things: ✨ If you are creating entirely new tests: Note; it is very important that the committed reference images are created on the build pipeline, locally created images are not reliable. Similarly tests will fail locally but you can look at the report to check they are "visually similar". See https://github.com/jMonkeyEngine/jmonkeyengine/blob/master/jme3-screenshot-tests/README.md for more information Contact @richardTingle (aka richtea) for guidance if required |
|
i think there is the possibility that this might cause issues for existing apps assuming filters run on quads, can you make this configurable? |
|
Yeah, i see the point, since the previous vertex shader did the position transform, any filter that does not use jme's stock vertex shader fails. Making the java side configurable should be quite easy. Have to think about the gl side. First i am going to explore why post water fails, then i look into making it configurable. |
…the post.vert transformations
|
I reverted the shaders to stock and modified the meshes positions to get the correct result after transformation. Very likely i am not able to fix that. Of course, once i add a switch to the fpp this issue is gone since i will default to the original quad i guess? |
|
Yes, better to default to the quad and leave this only as an optional performance setting. |
…cessor` rendering.
|
I added a secondary constructor allowing to switch on the triangle, not sure if allowing runtime switches trough a setter is a good idea because i remember i cloned that quad previously myself. But if you like that better then a second constructor i am fine with it too. |
|
the constructor is perfect, thanks |
|
ok, ill add some tests if wanted in the next days |
| private static final float[] POSITIONS = { | ||
| 0, 0, 0, // -1 -1 0 after vertex shader transform | ||
| 2, 0, 0, // 3 -1 0 after vertex shader transform | ||
| 0, 2, 0 // -1 3 0 after vertex shader transform |
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.
💯
Reference text on why it performs slightly better: https://wallisc.github.io/rendering/2021/04/18/Fullscreen-Pass.html
Maybe not worth it, but i noticed measurable results when using calculation heavy shaders
Just run the local tests to verify it is working.