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 missing textures in atlas. #3011

Merged
merged 10 commits into from
Sep 16, 2015
Merged

Fix missing textures in atlas. #3011

merged 10 commits into from
Sep 16, 2015

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Sep 9, 2015

Render to texture instead of copy from framebuffer on texture atlas resize. For #2997.

Still investigating GL calls for submitting a Chrome bug.

*
* @private
*/
RenderState.removeFromCache = function(renderState) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to reference count these, right? What if someone else created the same render state?

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 9, 2015

Update CHANGES.md.

framebuffer._bind();
newTexture.copyFromFramebuffer(0, 0, 0, 0, oldAtlasWidth, oldAtlasHeight);
command.execute(textureAtlas._context);
Copy link
Contributor

Choose a reason for hiding this comment

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

TextureAtlas is some of the oldest code in Cesium - it predates commands by at least a year.

I am not sure that TextureAtlas should keep a reference to context. No one else does that. This allows TextureAtlas to make WebGL calls whenever it wants regardless of the global state. This is not good for scheduling requestAnimFrame and for interop for other WebGL engines (#648). I wonder if it is even the source of this issue.

Perhaps we could replace _context with an update function and use ComputeCommands? How hard is that refactor? Is it too architect astronaut-ish?

@bagnell
Copy link
Contributor Author

bagnell commented Sep 15, 2015

@pjcozzi This is ready for another look.

I agree that TextureAtlas should have an update function, but we should create an issue and add it in the future.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 15, 2015

Works well and code looks good. Just add unit tests to RenderStateSpec.js.

@bagnell
Copy link
Contributor Author

bagnell commented Sep 16, 2015

@pjcozzi This is ready for another look. I changed how the states are reference counted. The partial keys are reference counted as before, but the full key reference count is now how many partial keys reference the full state.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 16, 2015

Looks good.

pjcozzi added a commit that referenced this pull request Sep 16, 2015
@pjcozzi pjcozzi merged commit 18a80ae into master Sep 16, 2015
@pjcozzi pjcozzi deleted the texture-atlas-resize branch September 16, 2015 18:58
@lasalvavida lasalvavida mentioned this pull request Apr 8, 2016
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.

2 participants