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.

Sign up for GitHub

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

Jump to bottom

feat(core): Explicitly set stepMode in Attribute layout #8858

Merged
merged 4 commits into from
May 6, 2024
Merged

feat(core): Explicitly set stepMode in Attribute layout #8858

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
602839d
Replace divisor with stepMode in Attribute
Pessimistress Apr 30, 2024
303a87b
docs
Pessimistress Apr 30, 2024
beda349
jsdoc
Pessimistress May 1, 2024
86b688b
Merge branch 'master' into x/instanced-attribute
Pessimistress May 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
46 changes: 28 additions & 18 deletions docs/api-reference/core/attribute-manager.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ the attributes that should be auto-calculated.
```js
attributeManager.add({
positions: {size: 2, accessor: 'getPosition', update: calculatePositions},
colors: {size: 4, type: GL.UNSIGNED_BYTE, accessor: 'getColor', update: calculateColors}
colors: {size: 4, type: 'unorm8', accessor: 'getColor', update: calculateColors}
});
```

Expand All @@ -61,13 +61,10 @@ Takes a single parameter as a map of attribute descriptor objects:
* keys are attribute names
* values are objects with attribute definitions:
- luma.gl [accessor parameters](https://luma.gl/docs/api-reference-legacy/classes/accessor):
* `type` (string, optional) - data type of the attribute, see "Remarks" section below.
* `type` (string, optional) - data type of the attribute, see "Remarks" section below. Default `'float32'`.
* `size` (number) - number of elements per vertex
* `normalized` (boolean) - default `false`
* `integer` (boolean) - WebGL2 only, default `false`
* `divisor` (boolean, optional) - `1` if this is an instanced attribute
(a.k.a. divisor). Default to `0`.
- deck.gl attribute configurations:
* `stepMode` (string, optional) - One of `'vertex'`, `'instance'` and `'dynamic'`. If set to `'dynamic'`, will be resolved to `'instance'` when this attribute is applied to an instanced model, and `'vertex'` otherwise. Default `'vertex'`.
* `isIndexed` (boolean, optional) - if this is an index attribute
(a.k.a. indices). Default to `false`.
* `accessor` (string | string[] | Function) - accessor name(s) that will
Expand All @@ -85,12 +82,10 @@ Takes a single parameter as a map of attribute descriptor objects:
* `size` (number) - number of elements per vertex
* `vertexOffset` (number) - offset of the attribute by vertex (stride). Default `0`.
* `elementOffset` (number) - offset of the attribute by element. default `0`.
* `divisor` (boolean, optional) - `1` if this is an instanced attribute
(a.k.a. divisor). Default to `0`.

#### `addInstanced` {#addinstanced}

Shorthand for `add()` in which all attributes `instanced` field are set to `true`.
Shorthand for `add()` in which all attributes `stepMode` field are set to `'instance'`.


#### `remove` {#remove}
Expand Down Expand Up @@ -152,6 +147,16 @@ Notes:
* Any preallocated buffers in "buffers" matching registered attribute names will be used. No update will happen in this case.
* Calls onUpdateStart and onUpdateEnd log callbacks before and after.

#### `getBufferLayouts`

Returns WebGPU-style buffer layout descriptors.

Parameters:

* `modelInfo` (object) - a luma.gl `Model` or a similarly shaped object
+ `isInstanced` (boolean) - used to resolve `stepMode: 'dynamic'`


## Remarks

### Attribute Type
Expand All @@ -160,15 +165,20 @@ The following `type` values are supported for attribute definitions:

| type | value array type | notes |
| ---- | ---------------- | ----- |
| `GL.FLOAT` | `Float32Array` | |
| `GL.DOUBLE` | `Float64Array` | Because 64-bit floats are not supported by WebGL, the value is converted to an interleaved `Float32Array` before uploading to the GPU. It is exposed to the vertex shader as two attributes, `<attribute_name>` and `<attribute_name>64Low`, the sum of which is the 64-bit value. |
| `GL.BYTE` | `Int8Array` | |
| `GL.SHORT` | `Int16Array` | |
| `GL.INT` | `Int32Array` | |
| `GL.UNSIGNED_BYTE` | `Uint8ClampedArray` | |
| `GL.UNSIGNED_SHORT` | `Uint16Array` | |
| `GL.UNSIGNED_INT` | `Uint32Array` | |

| `float32` | `Float32Array` | |
| `float64` | `Float64Array` | Because 64-bit floats are not supported by WebGL, the value is converted to an interleaved `Float32Array` before uploading to the GPU. It is exposed to the vertex shader as two attributes, `<attribute_name>` and `<attribute_name>64Low`, the sum of which is the 64-bit value. |
| `sint8` | `Int8Array` | |
| `snorm8` | `Int8Array` | Normalized |
| `uint8` | `Uint8ClampedArray` | |
| `unorm8` | `Uint8ClampedArray` | Normalized |
| `sint16` | `Int16Array` | |
| `snorm16` | `Int16Array` | Normalized |
| `uint16` | `Uint16Array` | |
| `unorm16` | `Uint16Array` | Normalized |
| `sint32` | `Int32Array` | |
| `uint32` | `Uint32Array` | |


## Source

[modules/core/src/lib/attribute-manager.ts](https://github.com/visgl/deck.gl/blob/master/modules/core/src/lib/attribute/attribute-manager.ts)
1 change: 1 addition & 0 deletions docs/upgrade-guide.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ class MyLayer {
While the 9.0 release of deck.gl does not yet support WebGPU, our goal is to enable WebGPU soon in a 9.x release. A number of changes will be required to deck.gl curtom layers:

- deck.gl now uses uniform buffers instead of global uniforms. It is not yet required to use uniform buffers but it will be necessary if you would like to run deck.gl on WebGPU in future releases.
- When defining an attribute, `type` is now a WebGPU-style [string format](https://luma.gl/docs/api-guide/gpu/gpu-attributes#vertexformat) instead of GL constant, and `divisor` is replaced by `stepMode`. See [AttributeManager.add](./api-reference/core/attribute-manager.md#add)
- WebGL draw modes `GL.TRIANGLE_FAN` and `GL.LINE_LOOP` are not supported on WebGPU. Select a different topology when creating geometries.
- The luma picking module now [uses uniform buffers](https://github.com/visgl/luma.gl/blob/master/modules/shadertools/src/modules/engine/picking/picking.ts#L34-L50). To access picking state in shaders use `picking.isActive` rather than `picking_isActive`

Expand Down
56 changes: 25 additions & 31 deletions modules/core/src/lib/attribute/attribute-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ export default class AttributeManager {

// Adds attributes
addInstanced(attributes: {[id: string]: AttributeOptions}) {
this._add(attributes, {instanced: 1});
this._add(attributes, {stepMode: 'instance'});
}

/**
Expand Down Expand Up @@ -308,50 +308,44 @@ export default class AttributeManager {
return changedAttributes;
}

/** Generate WebGPU-style buffer layout descriptors from all attributes */
getBufferLayouts(
attributes?: {[id: string]: Attribute},
excludeAttributes: Record<string, boolean> = {}
): BufferLayout[] {
if (!attributes) {
attributes = this.getAttributes();
}
const bufferMaps: BufferLayout[] = [];
for (const attributeName in attributes) {
if (!excludeAttributes[attributeName]) {
bufferMaps.push(attributes[attributeName].getBufferLayout());
}
/** A luma.gl Model-shaped object that supplies additional hint to attribute resolution */
modelInfo?: {
/** Whether the model is instanced */
isInstanced?: boolean;
}
return bufferMaps;
): BufferLayout[] {
return Object.values(this.getAttributes()).map(attribute =>
attribute.getBufferLayout(modelInfo)
);
}

// PRIVATE METHODS

// Used to register an attribute
private _add(attributes: {[id: string]: AttributeOptions}, extraProps: any = {}) {
/** Register new attributes */
private _add(
/** A map from attribute name to attribute descriptors */
attributes: {[id: string]: AttributeOptions},
/** Additional attribute settings to pass to all attributes */
overrideOptions?: Partial<AttributeOptions>
) {
for (const attributeName in attributes) {
const attribute = attributes[attributeName];

const props: AttributeOptions = {
...attribute,
id: attributeName,
size: (attribute.isIndexed && 1) || attribute.size || 1,
...overrideOptions
};

// Initialize the attribute descriptor, with WebGL and metadata fields
this.attributes[attributeName] = this._createAttribute(attributeName, attribute, extraProps);
this.attributes[attributeName] = new Attribute(this.device, props);
}

this._mapUpdateTriggersToAttributes();
}
/* eslint-enable max-statements */

private _createAttribute(name: string, attribute: AttributeOptions, extraProps: any) {
// For expected default values see:
// https://github.com/visgl/luma.gl/blob/1affe21352e289eeaccee2a876865138858a765c/modules/webgl/src/classes/accessor.js#L5-L13
// and https://deck.gl/docs/api-reference/core/attribute-manager#add
const props: AttributeOptions = {
...attribute,
id: name,
size: (attribute.isIndexed && 1) || attribute.size || 1,
divisor: extraProps.instanced ? 1 : attribute.divisor || 0
};

return new Attribute(this.device, props);
}

// build updateTrigger name to attribute name mapping
private _mapUpdateTriggersToAttributes() {
Expand Down
19 changes: 16 additions & 3 deletions modules/core/src/lib/attribute/attribute.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ export type Updater = (

export type AttributeOptions = DataColumnOptions<{
transition?: boolean | Partial<TransitionSettings>;
stepMode?: 'vertex' | 'instance' | 'dynamic';
noAlloc?: boolean;
update?: Updater;
accessor?: Accessor<any, any> | string | string[];
Expand Down Expand Up @@ -363,19 +364,31 @@ export default class Attribute extends DataColumn<AttributeOptions, AttributeInt
return result;
}

getBufferLayout(): BufferLayout {
/** Generate WebGPU-style buffer layout descriptor from this attribute */
getBufferLayout(
/** A luma.gl Model-shaped object that supplies additional hint to attribute resolution */
modelInfo?: {isInstanced?: boolean}
Pessimistress marked this conversation as resolved.
Show resolved Hide resolved
): BufferLayout {
// Clear change flag
this.state.layoutChanged = false;

const shaderAttributeDefs = this.settings.shaderAttributes;
const result: BufferLayout = super.getBufferLayout();
const result: BufferLayout = super._getBufferLayout();
const {stepMode} = this.settings;
if (stepMode === 'dynamic') {
// If model info is provided, use isInstanced flag to determine step mode
// If no model info is provided, assume it's an instanced model (most common use case)
result.stepMode = modelInfo ? (modelInfo.isInstanced ? 'instance' : 'vertex') : 'instance';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it worth logging a warning here if falling back to assumptions? We generally want to reduce the amount of magic we rely on...

Or is it too common / convenient to let layers omit?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Every layer calls attributeManager.getBufferLayouts() when constructing their models. Even if the layer does not contain any attribute with stepMode: 'dynamic', it may be used with an extension that does. Unless you think getBufferLayouts should always be called with a isInstanced argument (which is quite verbose), I'd prefer a quiet fallback.

I am personally not a fan of the noisy luma warnings in v9. Users should not be shown warnings that they cannot do anything about. Log levels should be used for development purpose.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unless you think getBufferLayouts should always be called with a isInstanced argument (which is quite verbose), I'd prefer a quiet fallback.

Understood. For the longer term. I am suspecting that AttributeManager is perhaps doing too much. One the one had it is a binary table generator and on the other hand it is used to map that binary table to different attribute sets. I suspect that further separating these two steps at some point (e.g. remove addInstanced()) might make it easier to generate precise layouts for each model.That is, whether the binary columns in the table is instanced seems like a technicality that the binary table management shouldn't need to worry about, but should be applied when a layout is requested. So in this sense, all attributes should be dynamic.

I haven't touched this code in a long time so it would probably take some discussion to work it through.

I am personally not a fan of the noisy luma warnings in v9. Users should not be shown warnings that they cannot do anything about. Log levels should be used for development purpose.

Sure, whether those logs are shown at debug level 0 or level 1 is a reasonable separate discussion.

Though in my view that doesn't solve anything. Relying on warnings being suppressed doesn't seem right, and showing them in debug mode even though they are "intentional" isn't helpful either, so I still I feel that until we find a way for deck to generate more precise layouts, we still need options for deck to turn them off.

} else {
result.stepMode = stepMode ?? 'vertex';
}

if (!shaderAttributeDefs) {
return result;
}

for (const shaderAttributeName in shaderAttributeDefs) {
const map = super.getBufferLayout(
const map = super._getBufferLayout(
shaderAttributeName,
shaderAttributeDefs[shaderAttributeName]
);
Expand Down
4 changes: 1 addition & 3 deletions modules/core/src/lib/attribute/data-column.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@ export type BufferAccessor = {
type?: DataType;
/** The number of elements per vertex attribute. */
size?: number;
/** 1 if instanced. */
divisor?: 1 | 0;
/** Offset of the first vertex attribute into the buffer, in bytes. */
offset?: number;
/** The offset between the beginning of consecutive vertex attributes, in bytes. */
Expand Down Expand Up @@ -261,7 +259,7 @@ export default class DataColumn<Options, State> {
return result;
}

getBufferLayout(
protected _getBufferLayout(
attributeName: string = this.id,
options: Partial<ShaderAttributeOptions> | null = null
): BufferLayout {
Expand Down
2 changes: 1 addition & 1 deletion modules/core/src/lib/layer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -771,7 +771,7 @@ export default abstract class Layer<PropsT extends {} = {}> extends Component<
if (bufferLayoutChanged) {
// AttributeManager is always defined when this method is called
const attributeManager = this.getAttributeManager()!;
model.setBufferLayout(attributeManager.getBufferLayouts());
model.setBufferLayout(attributeManager.getBufferLayouts(model));
// All attributes must be reset after buffer layout change
changedAttributes = attributeManager.getAttributes();
}
Expand Down
11 changes: 2 additions & 9 deletions modules/extensions/src/brushing/brushing-extension.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,15 +69,8 @@ export default class BrushingExtension extends LayerExtension {
attributeManager.add({
brushingTargets: {
size: 2,
accessor: 'getBrushingTarget',
shaderAttributes: {
brushingTargets: {
divisor: 0
},
instanceBrushingTargets: {
divisor: 1
}
}
stepMode: 'dynamic',
Pessimistress marked this conversation as resolved.
Show resolved Hide resolved
accessor: 'getBrushingTarget'
}
});
}
Expand Down
8 changes: 0 additions & 8 deletions modules/extensions/src/brushing/shader-module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,7 @@ const vs = glsl`
uniform vec2 brushing_mousePos;
uniform float brushing_radius;

#ifdef NON_INSTANCED_MODEL
Pessimistress marked this conversation as resolved.
Show resolved Hide resolved
in vec2 brushingTargets;
#else
in vec2 instanceBrushingTargets;
#endif

out float brushing_isVisible;

Expand Down Expand Up @@ -89,11 +85,7 @@ const inject = {
} else if (brushing_target == 1) {
brushingTarget = geometry.worldPositionAlt.xy;
} else {
#ifdef NON_INSTANCED_MODEL
brushingTarget = brushingTargets;
#else
brushingTarget = instanceBrushingTargets;
#endif
}
bool visible;
if (brushing_target == 3) {
Expand Down
7 changes: 2 additions & 5 deletions modules/extensions/src/collision-filter/collision-filter-extension.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,8 @@ export default class CollisionFilterExtension extends LayerExtension {
attributeManager!.add({
collisionPriorities: {
size: 1,
accessor: 'getCollisionPriority',
shaderAttributes: {
collisionPriorities: {divisor: 0},
instanceCollisionPriorities: {divisor: 1}
}
stepMode: 'dynamic',
accessor: 'getCollisionPriority'
}
});
}
Expand Down
8 changes: 0 additions & 8 deletions modules/extensions/src/collision-filter/shader-module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,7 @@ import {project} from '@deck.gl/core';
import {glsl} from '../utils/syntax-tags';

const vs = glsl`
#ifdef NON_INSTANCED_MODEL
in float collisionPriorities;
#else
in float instanceCollisionPriorities;
#endif

uniform sampler2D collision_texture;
uniform bool collision_sort;
Expand Down Expand Up @@ -60,11 +56,7 @@ const inject = {
`,
'vs:DECKGL_FILTER_GL_POSITION': glsl`
if (collision_sort) {
#ifdef NON_INSTANCED_MODEL
float collisionPriority = collisionPriorities;
#else
float collisionPriority = instanceCollisionPriorities;
#endif
position.z = -0.001 * collisionPriority * position.w; // Support range -1000 -> 1000
}

Expand Down
22 changes: 4 additions & 18 deletions modules/extensions/src/data-filter/data-filter-extension.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,15 +167,8 @@ export default class DataFilterExtension extends LayerExtension<
filterValues: {
size: filterSize,
type: fp64 ? 'float64' : 'float32',
accessor: 'getFilterValue',
shaderAttributes: {
filterValues: {
divisor: 0
},
instanceFilterValues: {
divisor: 1
}
}
stepMode: 'dynamic',
accessor: 'getFilterValue'
}
});
}
Expand All @@ -184,19 +177,12 @@ export default class DataFilterExtension extends LayerExtension<
attributeManager.add({
filterCategoryValues: {
size: categorySize,
stepMode: 'dynamic',
accessor: 'getFilterCategory',
transform:
categorySize === 1
? d => extension._getCategoryKey.call(this, d, 0)
: d => d.map((x, i) => extension._getCategoryKey.call(this, x, i)),
shaderAttributes: {
filterCategoryValues: {
divisor: 0
},
instanceFilterCategoryValues: {
divisor: 1
}
}
: d => d.map((x, i) => extension._getCategoryKey.call(this, x, i))
}
});
}
Expand Down