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

Remove floating-point texture requirement #4700

Merged
merged 3 commits into from
Dec 1, 2016
Merged

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Nov 30, 2016

Add falling back to packing floats into unsigned byte texture when floating point textures aren't supported.

Fixes #4563.

@bagnell
Copy link
Contributor Author

bagnell commented Nov 30, 2016

I tested this on my Galaxy S6. Most newer devices with OpenGL ES 3.0 or older devices that do not have the OES_texture_float extension will reproduce the issue.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 30, 2016

@bagnell is there anything duplicate here that can be moved to a shared private utility file?

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 30, 2016

Can you also add a code comment about how this is a fallback and has some cases that are not memory efficient as we discussed offline?


offsets[i] = currentOffset;

if (componentDatatype !== ComponentDatatype.UNSIGNED_BYTE && packFloats) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be cleaner to do this check outside of the loop to assign to a stride variable then here just do: currentOffset += stride;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would increase the memory even more. If the type is unsigned byte, we still only use one texel, but, if its a larger type, then we pack into unsigned bytes which is 4 texels.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not all attributes will have the same component type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I missed that.

return '';
}

return 'float unpackFloat(vec4 value) \n' +
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a czm_ function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wouldn't move this because it mirrors unpackFloat. If we could use the packing/unpacking code elsewhere, I'd be OK with it. Can you think of another case where we would do something like this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like anywhere we need to emulate float textures, but this is not important enough to hold up the release.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 30, 2016

@bagnell I can do a final test and merge tomorrow morning before the release. Let me know when this is updated.

@bagnell
Copy link
Contributor Author

bagnell commented Nov 30, 2016

@bagnell is there anything duplicate here that can be moved to a shared private utility file?

The packing/unpacking code is similar to packing elsewhere but the only thing that would be shared is the constants.

@bagnell
Copy link
Contributor Author

bagnell commented Nov 30, 2016

@pjcozzi This is ready unless you want me to separate out the packing code.

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 1, 2016

Thanks @bagnell.

Can you merge master to 3d-tiles and fix all the calls to the BatchTable constructor?

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.

2 participants