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

Transform and state sorting reform #1827

Open
wants to merge 44 commits into
base: master
Choose a base branch
from

Conversation

NolaDonato
Copy link
Contributor

  1. Added capability to calculate matrices in a general way and put them in a transform blocfk
  2. Accumulated state sorting culling and transform code into a new RenderSorter class
  3. Custom state sorting for main scene, post effects and shadow maps
  4. Eliminated redundant GL state calls
  5. Accumulated all render state into a single 64 bit number
  6. Store render state in render pass
  7. Added transparent flag to ShaderData
  8. Added GVRRenderer class

GearVRF DCO signed off by: Nola Donato [email protected]

Not for merge yet - Vulkan is not working.

@liaxim
Copy link
Contributor

liaxim commented Apr 10, 2018

There are some commits adding pictures that do not belong here.

render_data_string.append(std::to_string(mesh_->getVertexBuffer()->getDescriptor()));
hash_code = render_data_string;
hash_code_dirty_ = false;
pass(0)->render_modes().clearDirty();
Copy link
Contributor

Choose a reason for hiding this comment

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

not for all the passes?

Copy link
Contributor

Choose a reason for hiding this comment

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

We have to talk about the hash code. All of the render states now fit in a single 64 bit word, one for each pass. The render passes are rendered separately, not together anymore. OpenGL doesn't use this hash code. We should discuss its significance in Vulkan.

@Caprica666
Copy link
Contributor

I removed the one picture that was left in the repo. How do I remove commits in the middle? I was in a hurry to finish Siggraph registration and they needed photos of a specific size for the badge. I used Git to transfer stuff between PC and Linux...

@liaxim
Copy link
Contributor

liaxim commented Apr 11, 2018

You could "git revert" the unwanted commits. Or use git interactive rebase - "git rebase -i HEAD~15" to drop those commits.

@liaxim
Copy link
Contributor

liaxim commented Apr 11, 2018

And needs a rebase

offset_factor = src.offset_factor;
offset_units = src.offset_units;
render_order = src.render_order;
sample_coverage = src.sample_coverage;
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing return; please compile with GVRF_USE_CLANG=true and address the warnings in the new and changed code.

@liaxim
Copy link
Contributor

liaxim commented Apr 11, 2018

gvr-accessibility doesn't look right:
1

@liaxim
Copy link
Contributor

liaxim commented Apr 11, 2018

gvr-controls:

2

@liaxim
Copy link
Contributor

liaxim commented Apr 11, 2018

gvr-immersivepedia

3

@liaxim
Copy link
Contributor

liaxim commented Apr 11, 2018

gvr-keyboard launches into a black screen

@liaxim
Copy link
Contributor

liaxim commented Apr 11, 2018

gvr-multilight - z-fighting?

@liaxim
Copy link
Contributor

liaxim commented Apr 11, 2018

gvr-modelviewer2 - looks weird, giant reticle

@liaxim
Copy link
Contributor

liaxim commented Apr 11, 2018

gvr-shadows doesn't look right as you look up and down

@liaxim
Copy link
Contributor

liaxim commented Apr 11, 2018

gvr-sceneobjects

4

@liaxim
Copy link
Contributor

liaxim commented Apr 11, 2018

gvr-simplephysics, gvr-switch, gvr-tutorial-lesson6, gvr-x3d-demo all don't work

@liaxim
Copy link
Contributor

liaxim commented Apr 11, 2018

Such a giant change needs to be backed up by automated tests proving correctness; or at least some level of. The previous thing was quite thoroughly tested over an extended period of time; I seriously doubt going through all the demo covers everything and can be considered enough for a change of this magnitude.

@liaxim
Copy link
Contributor

liaxim commented Apr 11, 2018

Ok, is there a Demos pull request that goes along with this one that is missing?

* @param isStereo true for stereo rendering, false for mono
* @returns number of matrices actually calculated and stored in the output buffer
*/
public static int calcMatrix(float[] inputMatrices, float[] outputMatrices, int numMatrices, boolean isStereo)
Copy link
Contributor

Choose a reason for hiding this comment

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

The calcMatrix methods on the Java side show up as unused. Are they needed?

@NolaDonato
Copy link
Contributor Author

PR #597 in GearVRf-Demos addresses some of these issues. I am debugging the others. I think there is an issue with render mask which is why gvr-sceneobjects is broken. I also took out the default of making everything use alpha blending. This is why some of the others are broken. We should not have this on by default.

gl_Position = u_mvp * posn;
#endif

gl_Position = u_mvp * posn;
Copy link
Contributor

Choose a reason for hiding this comment

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

The HAS_MULTIVIEW ifdef block is really not needed anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this is no longer necessary because the Java shader generation code takes care of it now.

@NolaDonato
Copy link
Contributor Author

There were some shader generation bugs causing some issues with the examples. I have fixed these. Now the immersivepedia, accessibility, shadows, controls, simplephysics, modelviewer2, keyboard, switch, tutorial , and x3ddemo samples work now.

@liaxim
Copy link
Contributor

liaxim commented May 30, 2018

It is unfortunate that this work cannot be done in more incremental way.

@roshanch
Copy link
Contributor

the reason I am saying it because efforts needed to rebase it. everyday we have to rebase it..

@liaxim
Copy link
Contributor

liaxim commented May 30, 2018

@roshanch I absolutely agree. This is why this work needs to be broken down and done in increments. Also this pull is of such size that it is effectively un-reviewable. Increments we can reason about and easier to test since you can tell what to target.

@NolaDonato
Copy link
Contributor Author

I just did the rebase. Since you have been doing it every day perhaps you can spot what I did wrong. Sushant and I are going to try to do the Vulkan part.

@NolaDonato
Copy link
Contributor Author

I have done a lot of testing and the problem is not a merge failure. The code works fine on a Note8 with Android O. It fails on a Note8 with Android N. It also fails on an S8 with Android O. It appears to corrupt uniform blocks. The lighting anomalies are caused by the view matrix being incorrect. Printing it right before the uniform block is loaded into the GPU shows no errors. Still investigating.

@sushantojal
Copy link
Contributor

The issue is with the GL_MAX_VERTEX_UNIFORM_COMPONENTS query. Looks like the value returned by the driver is not reliable. For gvr-multilight, any block of more than 18 matrices (288 floats) does not work for the S8+ (adreno, Android N) I am using; even though the query returns 1024 (64 mats). Works fine for values 18 and below.
Similar issues have existed for other drivers, but I wasnt able to find any documentation confirming if this is a known issue on this particular driver version.
We'll need to come up with a reliable workaround for this.

@liaxim
Copy link
Contributor

liaxim commented Nov 28, 2018

Should we close?

@NolaDonato
Copy link
Contributor Author

No I am going to revive this one

@liaxim
Copy link
Contributor

liaxim commented Nov 29, 2018

Revive against GearVRf instead of SXRSDK?!

@NolaDonato
Copy link
Contributor Author

That is easier to get working I think. Once it works with GearVRf I can rename it and do a PR against SXR.

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

Successfully merging this pull request may close these issues.

5 participants