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

Metadata picking preparation #12075

Open
wants to merge 45 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
656c638
Removed some unused properties
javagl Jul 9, 2024
247102f
Draft of metadata picking
javagl Jul 9, 2024
e561dec
Fix typo
javagl Jul 9, 2024
cc1b826
Try to avoid recompilation
javagl Jul 15, 2024
a28f62d
Do not modify the bounding sphere...
javagl Jul 16, 2024
122c4e8
Remove debug stuff
javagl Jul 16, 2024
a44ac07
Draft for warnings for unsupported property types
javagl Jul 17, 2024
e0c57f6
Draft for encoding metadata values for picking
javagl Jul 17, 2024
dc7552c
Update matrix only when command is defined
javagl Jul 19, 2024
609e277
Minor comment fix
javagl Jul 19, 2024
927d01b
Draft for metadata value decoding
javagl Jul 19, 2024
461821b
Minor consolidation
javagl Jul 20, 2024
261dbba
Fix JSDoc type
javagl Jul 25, 2024
3817389
Do not duplicate full command - use derived command
javagl Jul 25, 2024
a183fd4
Merge remote-tracking branch 'origin/main' into metadata-picking-prep…
javagl Jul 25, 2024
7848f5d
Minor consolidation
javagl Jul 30, 2024
66b3252
Update ModelRuntimePrimitiveSpec
javagl Jul 30, 2024
016a1a8
Fix import file extension
javagl Jul 30, 2024
d575317
Trying to make JSDoc happier
javagl Jul 31, 2024
4d0b722
Merge remote-tracking branch 'origin/main' into metadata-picking-prep…
javagl Jul 31, 2024
2f29019
Remove debug log
javagl Aug 18, 2024
a1989c0
First pass of PR feedback
javagl Aug 18, 2024
88500ec
Removed debug log
javagl Aug 18, 2024
fe4c2bd
Replace repeated strings with constants
javagl Aug 18, 2024
f8bac8f
Generalizations and cleanups for metadata decoding
javagl Aug 23, 2024
8a24da2
Hacky spec experiments
javagl Aug 28, 2024
ebddfe1
Revert some experiments. Cleanups.
javagl Aug 29, 2024
c0eaf7b
Remove debug log
javagl Aug 29, 2024
e732550
Cleanups for metadata picking specs
javagl Aug 29, 2024
775113d
Basic specs and debugging
javagl Aug 30, 2024
2056da0
Extend metadata picking specs
javagl Aug 31, 2024
0e6b7f6
Remove debug output
javagl Aug 31, 2024
45727e8
Remove inlined image creation
javagl Sep 7, 2024
2a948cf
Merge remote-tracking branch 'origin/main' into metadata-picking-prep…
javagl Sep 8, 2024
ddd5fb8
Wrap up specs for metadata picking
javagl Sep 10, 2024
4347a23
Merge remote-tracking branch 'origin/main' into metadata-picking-prep…
javagl Sep 10, 2024
d813dba
Update spec for new pipeline stage
javagl Sep 10, 2024
0ad2bc5
Revive parts of the metadata picking specs
javagl Sep 10, 2024
aa75922
Try declaring metadata picking specs as not-WebGL
javagl Sep 10, 2024
9c3c081
Classify metadata picking spec as WebGL
javagl Sep 11, 2024
c854235
Create WebGL 1 context for metadata picking specs
javagl Sep 11, 2024
384b2fa
Skip metadata picking specs on WebGL stubs
javagl Sep 11, 2024
3c1b497
Make sure that scene is defined in specs
javagl Sep 11, 2024
a005521
Do not try to remove the primitives
javagl Sep 11, 2024
30e80b1
Do not try to keep the scene
javagl Sep 11, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 39 additions & 1 deletion packages/engine/Source/Renderer/DrawCommand.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ function DrawCommand(options) {
this._owner = options.owner;
this._debugOverlappingFrustums = 0;
this._pickId = options.pickId;
this._pickMetadataAllowed = options.pickMetadataAllowed === true;
this._pickedMetadataInfo = undefined;

// Set initial flags.
this._flags = 0;
Expand Down Expand Up @@ -511,7 +513,7 @@ Object.defineProperties(DrawCommand.prototype, {
* during the pick pass.
*
* @memberof DrawCommand.prototype
* @type {string}
* @type {string|undefined}
* @default undefined
*/
pickId: {
Expand All @@ -525,6 +527,40 @@ Object.defineProperties(DrawCommand.prototype, {
}
},
},

/**
* Whether metadata picking is allowed
*
* @memberof DrawCommand.prototype
* @type {boolean}
* @default undefined
* @private
*/
pickMetadataAllowed: {
get: function () {
return this._pickMetadataAllowed;
},
},

/**
* Information about picked metadata.
*
* @memberof DrawCommand.prototype
* @type {PickedMetadataInfo|undefined}
* @default undefined
*/
pickedMetadataInfo: {
get: function () {
return this._pickedMetadataInfo;
},
set: function (value) {
if (this._pickedMetadataInfo !== value) {
this._pickedMetadataInfo = value;
this.dirty = true;
}
},
},

/**
* Whether this command should be executed in the pick pass only.
*
Expand Down Expand Up @@ -590,6 +626,8 @@ DrawCommand.shallowClone = function (command, result) {
result._owner = command._owner;
result._debugOverlappingFrustums = command._debugOverlappingFrustums;
result._pickId = command._pickId;
result._pickMetadataAllowed = command._pickMetadataAllowed;
result._pickedMetadataInfo = command._pickedMetadataInfo;
result._flags = command._flags;

result.dirty = true;
Expand Down
174 changes: 174 additions & 0 deletions packages/engine/Source/Scene/DerivedCommand.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import defined from "../Core/defined.js";
import DrawCommand from "../Renderer/DrawCommand.js";
import RenderState from "../Renderer/RenderState.js";
import ShaderSource from "../Renderer/ShaderSource.js";
import MetadataType from "./MetadataType.js";

/**
* @private
Expand Down Expand Up @@ -375,6 +376,179 @@ DerivedCommand.createPickDerivedCommand = function (
return result;
};

/**
* @private
*/
function replaceDefine(defines, defineName, newDefineValue) {
const n = defines.length;
for (let i = 0; i < n; i++) {
if (defines[i].startsWith(defineName)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we'll need a more precise regex here. There could be 2 defines starting with the same word, e.g., PICKING and PICKING_VOXEL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is now checking whether the first "token" in that define directive is the defineName (where "token" is everything before the first whitespace, trimmed).

defines[i] = `${defineName} ${newDefineValue}`;
}
}
}

/**
* @private
*/
function getComponentCount(classProperty) {
if (!classProperty.isArray) {
return MetadataType.getComponentCount(classProperty.type);
}
return classProperty.arrayLength;
}
function getGlslType(classProperty) {
const componentCount = getComponentCount(classProperty);
if (classProperty.normalized) {
if (componentCount === 1) {
return "float";
}
return `vec${componentCount}`;
}
if (componentCount === 1) {
return "int";
}
return `ivec${componentCount}`;
}

/**
* @private
*/
function getPickMetadataShaderProgram(
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be easier to read with some types in the doc. I'm assuming shaderProgram is a ShaderProgram ? Some confirmation would help me read it more confidently & quickly.

Copy link
Contributor Author

@javagl javagl Aug 18, 2024

Choose a reason for hiding this comment

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

Would a @see getPickShaderProgram be enough here? 😁

Seriously: I'm a HUGE fan of the "Boy Scout Rule" of programming:

Leave your code better than you found it.

Derived from the rule "Leave the campground cleaner that you found it". Now one could argue in how far this applies when boy scouts visit something that isn't a "campground" to begin with.

However, I added the information that the shaderProgram is a ShaderProgram. (This should be self-evident, but isn't (hint: a drawCommand is not always a DrawCommand)). I'd like to explain more, but explaining requires a depth of understanding that I won't claim to have.

context,
shaderProgram,
pickedMetadataInfo
) {
const schemaId = pickedMetadataInfo.schemaId;
const className = pickedMetadataInfo.className;
const propertyName = pickedMetadataInfo.propertyName;
const keyword = `pickMetadata-${schemaId}-${className}-${propertyName}`;
let shader = context.shaderCache.getDerivedShaderProgram(
shaderProgram,
keyword
);
if (defined(shader)) {
return shader;
}

const classProperty = pickedMetadataInfo.classProperty;
const glslType = getGlslType(classProperty);

// Define the components that will go into the output `metadataValues`.
// By default, all of them are 0.0.
const sourceValueStrings = ["0.0", "0.0", "0.0", "0.0"];
const componentCount = getComponentCount(classProperty);
if (componentCount === 1) {
// When the property is a scalar, store its value directly
// in `metadataValues.x`
sourceValueStrings[0] = `float(value)`;
} else {
// When the property is an array, store the array elements
// in `metadataValues.x/y/z/w`
const components = ["x", "y", "z", "w"];
for (let i = 0; i < componentCount; i++) {
const component = components[i];
const valueString = `value.${component}`;
sourceValueStrings[i] = `float(${valueString})`;
}
}

// Make sure that the `metadataValues` components are all in
// the range [0, 1] (which will result in RGBA components
// in [0, 255] during rendering)
if (!classProperty.normalized) {
for (let i = 0; i < componentCount; i++) {
sourceValueStrings[i] += " / 255.0";
}
}

let fs = shaderProgram.fragmentShaderSource;
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is common in existing code, but a two-letter variable name is not really our current recommendation. It's "short" but not "descriptive" 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the code to use const where it makes sense, and renamed it newFragmentShaderSource.
(I'm trying to cope with the tension between "following best practices" and "being consistent"...)

const newDefines = [...fs.defines];
Copy link
Contributor

Choose a reason for hiding this comment

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

I think shaderProgram.fragmentShaderSource.defines.slice() would be faster than a spread, and also avoid the mental overhead of a long-scoped let variable that's reassigned later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. (After additionally verifying that defines should never be undefined 🤞 )

newDefines.push("METADATA_PICKING_ENABLED");

// Replace the defines of the shader, using the type, property
// access, and value components that have been determined
replaceDefine(newDefines, "METADATA_PICKING_VALUE_TYPE", glslType);
replaceDefine(
newDefines,
"METADATA_PICKING_VALUE_STRING",
`metadata.${propertyName}`
);
replaceDefine(
newDefines,
"METADATA_PICKING_VALUE_COMPONENT_X",
sourceValueStrings[0]
);
replaceDefine(
newDefines,
"METADATA_PICKING_VALUE_COMPONENT_Y",
sourceValueStrings[1]
);
replaceDefine(
newDefines,
"METADATA_PICKING_VALUE_COMPONENT_Z",
sourceValueStrings[2]
);
replaceDefine(
newDefines,
"METADATA_PICKING_VALUE_COMPONENT_W",
sourceValueStrings[3]
);

// XXX_METADATA_PICKING
//*/
console.log("newDefines");
for (const d of newDefines) {
console.log(d);
}
//*/
fs = new ShaderSource({
Copy link
Contributor

Choose a reason for hiding this comment

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

So far, all we used from the old fs was the defines?
I think const newFragmentShaderSource would be clearer here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, it's newFragmentShaderSource now

sources: fs.sources,
defines: newDefines,
});
shader = context.shaderCache.createDerivedShaderProgram(
Copy link
Contributor

Choose a reason for hiding this comment

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

Here again, the previous use of this variable could be const existingShader, and this one const derivedShader

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, called it newShader to follow the new... pattern.

shaderProgram,
keyword,
{
vertexShaderSource: shaderProgram.vertexShaderSource,
fragmentShaderSource: fs,
attributeLocations: shaderProgram._attributeLocations,
}
);
return shader;
}

/**
* @private
*/
DerivedCommand.createPickMetadataDerivedCommand = function (
scene,
command,
context,
result
) {
if (!defined(result)) {
result = {};
}
result.pickMetadataCommand = DrawCommand.shallowClone(
command,
result.pickMetadataCommand
);

result.pickMetadataCommand.shaderProgram = getPickMetadataShaderProgram(
context,
command.shaderProgram,
command.pickedMetadataInfo
);
result.pickMetadataCommand.renderState = getPickRenderState(
scene,
command.renderState
);
result.shaderProgramId = command.shaderProgram.id;

return result;
};

function getHdrShaderProgram(context, shaderProgram) {
let shader = context.shaderCache.getDerivedShaderProgram(
shaderProgram,
Expand Down
31 changes: 31 additions & 0 deletions packages/engine/Source/Scene/FrameState.js
Original file line number Diff line number Diff line change
Expand Up @@ -421,6 +421,37 @@ function FrameState(context, creditDisplay, jobScheduler) {
* @default 0.0
*/
this.minimumTerrainHeight = 0.0;

/**
* Whether metadata picking is currently in progress.
*
* This is set to `true` in the `Picking.pickMetadata` function,
* immediately before updating and executing the draw commands,
* and set back to `false` immediately afterwards. It will be
* used to determine whether the metadata picking draw commands
* should be executed, in the `Scene.executeCommand` function.
*
* @type {boolean}
* @default false
*/
this.pickingMetadata = false;

/**
* Metadata picking information.
*
* This describes the metadata property that is supposed to be picked
* in a `Picking.pickMetadata` call.
*
* This is stored in the frame state and in the metadata picking draw
* commands. In the `Scene.updateDerivedCommands` call, it will be
* checked whether the instance that is stored in the frame state
* is different from the one in the draw command, and if necessary,
* the derived commands for metadata picking will be updated based
* on this information.
*
* @type {PickedMetadataInfo|undefined}
*/
this.pickedMetadataInfo = undefined;
}

/**
Expand Down
105 changes: 105 additions & 0 deletions packages/engine/Source/Scene/MetadataPicking.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
import MetadataComponentType from "./MetadataComponentType.js";

/**
* Utility functions for metadata picking.
*
* These are used by the `Picking.pickMetadata` function to decode
* the metadata values that have been read from the frame buffer
* into the actual metadata values, according to the structure
* defined by the `MetadataClassProperty`.
*
* This is marked as 'private', but supposed to be used in Picking.js.
*
* @private
*/
function MetadataPicking() {}

/**
* Returns the value at the specified inded of the given data view,
* interpreting the data to have the given component type.
*
* @param {MetadataComponentType} componentType The `MetadataComponentType`
* @param {DataView} dataView The data view
* @param {number} index The index
* @returns {number|bigint|undefined} The value
*
* @private
*/
MetadataPicking.decodeMetadataValue = function (
componentType,
dataView,
index
) {
switch (componentType) {
case MetadataComponentType.INT8:
return dataView.getInt8(index);
case MetadataComponentType.UINT8:
return dataView.getUint8(index);
case MetadataComponentType.INT16:
return dataView.getInt16(index);
case MetadataComponentType.UINT16:
return dataView.getUint16(index);
case MetadataComponentType.INT32:
return dataView.getInt32(index);
case MetadataComponentType.UINT32:
return dataView.getUint32(index);
case MetadataComponentType.INT64:
return dataView.getBigInt64(index);
case MetadataComponentType.UINT64:
return dataView.getBigUint64(index);
case MetadataComponentType.FLOAT32:
return dataView.getFloat32(index);
case MetadataComponentType.FLOAT64:
return dataView.getFloat64(index);
}
// Appropriate error handling?
return undefined;
};

/**
* Decode the given raw values into a metadata property value.
*
* The given values are a `Uint8Array` containing the RGBA
* values that have been read from the metadata picking
* frame buffer. They are assumed to contain the value for
* the given class property, as encoded by the
* `ModelDrawCommands` for metadata picking.
*
* @param {MetadataClassProperty} classProperty The `MetadataClassProperty`
* @param {Uint8Array} rawValues The raw values
* @returns {number|bigint|undefined} The value
*
* @private
*/
MetadataPicking.decodeMetadataValues = function (classProperty, rawValues) {
const componentType = classProperty.componentType;
const dataView = new DataView(
rawValues.buffer,
rawValues.byteOffset,
rawValues.byteLength
);
if (classProperty.isArray) {
const arrayLength = classProperty.arrayLength;
const result = Array(arrayLength);
for (let i = 0; i < arrayLength; i++) {
const element = MetadataPicking.decodeMetadataValue(
componentType,
dataView,
i
);
if (classProperty.normalized) {
result[i] = element / 255.0;
} else {
result[i] = element;
}
}
return result;
}
const value = MetadataPicking.decodeMetadataValue(componentType, dataView, 0);
if (classProperty.normalized) {
return value / 255.0;
}
return value;
};

export default Object.freeze(MetadataPicking);
Loading