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

Missing Textures Fix #3823

Merged
merged 13 commits into from
May 12, 2016
Merged

Missing Textures Fix #3823

merged 13 commits into from
May 12, 2016

Conversation

lasalvavida
Copy link
Contributor

This is a fix for #3730, though technically it is also a fix for #2997. It seems like #3730 was actually caused by #3011 breaking the resizing behavior of the TextureAtlas.

I included the contents of the TextureAtlas in the top left corner of the screenshots here to illustrate the failures.

The symptom of #2997 was the first letter disappearing.
missing-letters

The cause of this issue was actually the creation of the initial texture in the constructor. The font-size in the reported issue is important to note, because if the image being added was larger than the initial texture, it was re-generated and everything was fine.
#3011 fixed this, but caused issues when textures needed to be resized.

The resize failure can be seen below.

Before resize:
before-resize

After resize:
after-resize

I suspect the underlying cause is something to do with the state when the constructor is called. This can be resolved by creating the initial texture when the first image is added.

Here are the examples shown with the fix applied:
fixed1
fixed2

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 11, 2016

Thanks for looking into this, @lasalvavida.

Update CHANGES.md.

Also add a unit test if a reasonable one can be written for this.

@bagnell please review and merge when you are available.

@bagnell
Copy link
Contributor

bagnell commented Apr 11, 2016

@lasalvavida Are you seeing any test failures?

@lasalvavida
Copy link
Contributor Author

lasalvavida commented Apr 11, 2016

@bagnell, yes I am. Since the TextureAtlas doesn't rely on a shader + draw command anymore some of the color tests are failing and initial size is only respected when it is greater than the first image added with border, so some of those hard-coded size tests are failing. I'll update the tests.

Edit: The color test failures were caused by an issue in chrome where the texture wasn't respecting the firsttexSubImage2d call. I worked around it by explicitly setting the color on the texture.

@lasalvavida
Copy link
Contributor Author

@bagnell Whenever you get a chance, this is ready for a look

@mramato
Copy link
Contributor

mramato commented Apr 25, 2016

@lasalvavida can you merge in master and fix the conflicts (it's probably just changes.md)

@bagnell it would be nice to get this into the next release. Can you take a quick look when it's ready?

@lasalvavida
Copy link
Contributor Author

Merged changes.md

@bagnell
Copy link
Contributor

bagnell commented Apr 26, 2016

I still see 19 test failures in Chrome 50 that don't occur in master.

@lasalvavida
Copy link
Contributor Author

@bagnell, test failures should be fixed, and #3730 should no longer be present in @TomPed's example code

@lasalvavida
Copy link
Contributor Author

lasalvavida commented Apr 26, 2016

The first screenshot is from this CZML example which also seems to behave correctly.

gl.texImage2D(textureTarget, 0, internalFormat, width, height, 0, pixelFormat, pixelDatatype, null);
gl.framebufferTexture2D(framebufferTarget, gl.COLOR_ATTACHMENT0, textureTarget, texture, 0);

if(pixelFormat === gl.RGBA) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change this up a bit so that we attach to a framebuffer and clear only for RGBA textures?

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this only happen for RGBA textures? What about RGB textures? I doubt we would have the problem with any of the depth formats since they are only used as render targets.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you create a standalone example and submit a Chrome bug then add a link in the comment?

@lasalvavida
Copy link
Contributor Author

@bagnell The standalone test that I had put together no longer breaks in chrome. I reverted my changes to Texture.js as this no longer appears to be an issue.

The changes to TextureAtlas are still needed to fix #3730

@bagnell
Copy link
Contributor

bagnell commented May 12, 2016

@lasalvavida This looks good. Resolve the conflict with master and I'll merge this.

@lasalvavida
Copy link
Contributor Author

@bagnell Updated

@bagnell bagnell merged commit ca88219 into CesiumGS:master May 12, 2016
@bagnell bagnell mentioned this pull request Aug 26, 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.

4 participants