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

feat(engine) Support all vertex format types in GPUGeometry #1884

Merged
merged 5 commits into from
Dec 19, 2023

Conversation

felixpalmer
Copy link
Collaborator

For #1847

Background

GPUGeometry assumed that the buffer layout for attributes passed would always be using float32. This is wrong, for example when passing a Uint8Array for colors

Change List

  • Add getVertexFormatFromAttribute util funtion
  • Update GPUGeometry
  • Tests

@felixpalmer felixpalmer changed the title featu(engine) Support all vertex format types in GPUGeometry feat(engine) Support all vertex format types in GPUGeometry Dec 18, 2023
Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

I would prefer if the method that converts array types to data types was separated out into its own function.

getDataTypeFromArray()

We also have similar code in webgl/src/classic/typed-array-utils.ts it would be nice to rewrite that code in terms of those utilities calling these utilities so we only have such switch statements in one place.

In that code I switch on the constructor rather than doing instanceof, I have a hand-wavy sense that instanceof might be slower, but I haven't benched it.

Ideally I think these utilities should have "inverses" so that we can go both ways, and the names and types should be aligned.

@ibgreen
Copy link
Collaborator

ibgreen commented Dec 18, 2023

While I agree with these messages, we should also realize that as nice as it is to add helpful messages to developers, every such string gets bundled into every application built on luma.gl - however as they are just there to speed up debugging, in production applications they would never ever get used.

Because of this, my strategy is usually to write the helpful message in a comment just before the throw clause. Then I throw the minimal string I can, often just an input string. It does not take long for the developer to turn on "break on exceptions", see where luma throws and read the comment.

@felixpalmer felixpalmer merged commit 68e6515 into master Dec 19, 2023
2 checks passed
@felixpalmer felixpalmer deleted the felix/vertex-format-attribute branch December 19, 2023 10:30
Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

All good, just adding some notes after final read through.

@@ -9,6 +9,7 @@ const ERR_TYPE_DEDUCTION = 'Failed to deduce GL constant from typed array';
/**
* Converts TYPED ARRAYS to corresponding GL constant
* Used to auto deduce gl parameter types
* @deprecated Use getDataTypeFromTypedArray
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess one would need two functions: getDataTypeFromTypedArray and getGLFromVertexType

Note to self: "Vertex Type" being "Normalized Data Type" which is indeed confusing - might want to consider renaming VertexType to NormalizedDataType

@@ -40,6 +41,7 @@ export function getGLTypeFromTypedArray(arrayOrType: TypedArray): GLDataType {
/**
* Converts GL constant to corresponding TYPED ARRAY
* Used to auto deduce gl parameter types
* @deprecated Use getTypedArrayFromDataType
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similarly, one would need the getVertexTypeFromGL

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.

3 participants