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

Fix #1749 by adding an OIT flag. #2120

Merged
merged 7 commits into from Sep 9, 2014
Merged

Fix #1749 by adding an OIT flag. #2120

merged 7 commits into from Sep 9, 2014

Conversation

emackey
Copy link
Contributor

@emackey emackey commented Sep 8, 2014

Turns out OIT can be easily turned on and off at runtime, for example picking temporarily disables it.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 8, 2014

Thanks @emackey. Can you update README.md, add a test, and rename to orderIndependentTranslucency so it is consistent with fxaaOrderIndependentTranslucency, fxaa, and friends?

@bagnell
Copy link
Contributor

bagnell commented Sep 8, 2014

OIT uses a few textures and framebuffers. You may want to create/destroy it to create/release those resources.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 8, 2014

If this requires anything but completely trivial code, let's do what I suggested in #1749 since there is no value in being able to do this at runtime other than for a tech demo that we already have.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 8, 2014

Regardless, this should get pass through the constructors too.

@emackey
Copy link
Contributor Author

emackey commented Sep 8, 2014

This is moved to the constructors to save resources. Setting to true does not guarantee OIT of course, there are other factors at play, some of them per-frame. Do we need a property to expose whether or not OIT was used on a given frame?

Also, our SceneSpec does not test any of the construction options. It uses an external helper function to construct test scenes with test-suite construction options.

By README.md, did you mean CHANGES.md?

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 9, 2014

Ah, yes, CHANGES.md.

Do we need a property to expose whether or not OIT was used on a given frame?

Wouldn't worry about it for now.

Tests like these would be OK:

https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Specs/Widgets/CesiumWidget/CesiumWidgetSpec.js#L154

https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Specs/Widgets/Viewer/ViewerSpec.js#L285

We could also add a rendering test with a spy to ensure it disables OIT (assuming the system supports OIT) but it is borderline overkill.

Also, I want to make a few tweaks to cleanup the logic but I won't be able to get to it until next week after FOSS4G.

@emackey
Copy link
Contributor Author

emackey commented Sep 9, 2014

Tests added, other comments addressed. @pjcozzi just tell me what cleanup is needed and I'll clean it up, or open a separate PR after FOSS4G, but please don't let another branch go stale.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 9, 2014

This still needs a readonly like scene3DOnly. I also don't know why the check for options.orderIndependentTranslucency is so complicated, undefined will result in false, right? Or use defaultValue.

Bare with me on the review as I am on travel working around the clock for us.

@emackey
Copy link
Contributor Author

emackey commented Sep 9, 2014

Great suggestions, thanks. Code is cleaner as a result.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 9, 2014

Thanks!

pjcozzi added a commit that referenced this pull request Sep 9, 2014
@pjcozzi pjcozzi merged commit 11cd61e into master Sep 9, 2014
@pjcozzi pjcozzi deleted the oit-flag branch September 9, 2014 16:11
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.

None yet

3 participants