-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
base: main
Are you sure you want to change the base?
Metadata picking preparation #12075
Conversation
Thank you for the pull request, @javagl! ✅ We can confirm we have a CLA on file for you. |
I think the overall approach makes sense, although perhaps @lilleyse should chime in. My main concern: calling We only need to rebuild the shader when these values change: const schemaId = frameState.pickedMetadataSchemaId;
const className = frameState.pickedMetadataClassName;
const propertyName = frameState.pickedMetadataPropertyName; Thinking about how an end user would pick: there would be some UI element letting them choose a class and property. These values would change only rarely--once every few seconds, at most. Then, once they mouse over the model, A faster approach might be:
Note: you will need to keep 2 compiled versions of the shader alive: one for the rendering pass, and one for the metadata picking pass. See The compiled shaders are stored on
Then for each render pass, you need to select the appropriate |
I expected the
I have seen the The answer may be "In a DrawCommand". But ... there are reasons why I did not try the DrawCommand-based approach. (Well, in fact, I did try it - this PR is only the condensed tip of an iceberg of internal work and experiments...). One reason is that the code is ... let's call it intimidating. I roughly understand the idea behind DrawCommands. And I did start with some Another reason is that there apparently aren't really "draw commands" for the model - at least, not on the same level as for other elements: There's the However, I'll try to read more, try to map your recommendations to the code, and see whether there is a way to achieve that pseudocode-ish BTW: I've seen in your voxel picking PR that you started adding comments. That's good. When reading though long, undocumented functions, I usually cannot help myself, and try to make things easier to digest - even though this will likely be in vain... |
Only a short update: Creating different draw commands for the picking/rendering case of a model is not as simple as it might be in other cases. As mentioned above: There aren't really "draw commands" for the model. There is the Trying to ignore the details (because there are waaaay too many, too undocumented details), one approach that I tried was to add a
Ignoring 1., I'll first try to "do whatever is done to the |
Another update. This can probably be ignored. It is rather a form of "Rubber Duck Debugging" right now. The For each The At some point, it does build a tl;dr: The
Now, there's probably a point where cloning is not necessary. But given that undocumented modifications of undocumented properties are smeared all over the code, and nobody really knows what has to be cloned or not, the only reasonable approach for me right now is to try and clone things "until it works". An anecdote: Many years ago, I created a small "renderer" library. And I did also use the concept of a (Draw)Command there. It was a tad simpler, though. |
I wrote this two days ago. And still, it took me several hours to find out why ~"sometimes, under certain conditions, the picking frame buffer was not filled with proper values". Right now, it seems like this is indeed related to the fact that the render resources - specifically, the
indicates that it "works" when saving and re-assigning that Now...was that modification intentional? Does it serve a purpose? Should I avoid this modification? Does something else break when I prevent this modification? Can someone add a comment to the line This code is in dire need of cleanups. Having to hunt "bugs" that are caused by things like this will let any form of actual development come to a grinding halt. |
That bounding sphere modification. Sigh.
However, here's a small update:
Technically, this seems to work in principle, with the caveats:
An aside: When picking the metadata values with |
The last commit contains a draft for how the "encoding and decoding" of metadata values into the frame buffer could be handled. It is really only a draft, to quickly sync on the overall approach.
The values are still returned as a In the meantime, I'm less convinced of the overall approach than I was in the beginning. Funneling "arbitrary values" through the frame buffer, using the whole DrawCommand-infrastructure, feels like a "detour" (not to mention that it is a debugging nightmare). It would be great if there was an option to implement this more like pseudocode
I even considered to just ditch the whole
So basically really assign a What's also concerning: There now are four properties in the |
Offline we talked about using the scene-level picking and derived command infrastructure instead of creating pick metadata commands in Model.
|
The place where the current "derived commands" for picking are created is in the There are a few questions related to that. Note that I'm NOT asking these questions. Nobody has to even consider answering them. I'm only mentioning them, to make clear that I'm spending about 99% of my time with questions like these:
|
Theoretically, this might be possible. It is possible to create a The problem is that at this point, only the But I'm pretty sure that I'm missing some point here, so I wanted to confirm whether "shader source code string manipulation with regex find-and-replace" is indeed the intended solution for this. |
In the previous state, the approach was
This duplication was probably "too high", so to speak, and did lead to some issues. It's hard to tell for sure what exactly was duplicated there to begin with. And it's hard to say where the The last commit changes this, based on the recommendation by @lilleyse , to do the metadata handling in derived commands. The current state contains several 'markers', specifically, There now is a
but all the parts that are varying are represented as In the That derived command is created by
These values end up in the frame buffer, from where they are read, and translated back into the actual metadata value (a 2-element array in the above example). Now. Funneling that data through the frame buffer seems quirky. Assembling the proper shader for that based on Is this the intended approach? EDIT, a small note: Right now, this is still rebuilding the draw commands when the picked metadata property changes. Updating the "minimal set" (of derived commands?) upon changes still remains to be implemented... |
The last point of rebuilding the draw commands was now addressed as part of a minor ... "consolidation". From my point of view, the functionality that is added with this PR does not warrant the number and complexity of the changes that have been necessary to accomplish it. But doing what is necessary to decrease the complexity cannot be part of this PR. Maybe I'll add follow-up PRs similar to #12058 , but it's hard to make promises here. |
There already is (and has to be) the |
Yes. Treating 10 million points as "spheres" and finding the closest intersection with the picking ray usually does not make sense. But in view of the goal of picking metadata from a point cloud, there are a few "layers" below that, and I think that these layers make some considerations obsolete. E.g. how well does "picking individual points of a point cloud" work to begin with, unless each point has its own Ideally (or naively?) I'd hope that we can wrap this up to have that |
The issue of picking transparent parts has been resolved insofar that the places of an object that are transparent simply cannot be picked (even if this part might contain metadata). The last commits are attempts to provide some specs. The The camera is placed so that it exactly shows this square: And then, individual values can be picked from that. Right now, there is only one test case that picks some A caveat: When there is a property texture with a value of 127 in one channel, then this value will be converted to 127 / 255.0 = 0.498039216 in order to store it in one component of the |
Ouch. While writing the specs, and wondering why certain values did not make sense, I noticed that the 'alpha' topic is not completely resolved. An alpha value of 0 causes all values in the metadata picking frame buffer to become 0. Edit: This turned out to be wrong: It's not related to the metadata picking per se, but to metadata in general. Rendering a square with a custom shader, where the property texture is all white, but only the alpha component is 0, 64, 128, 255, shows that the actual metadata values are already affected by the alpha: When the alpha is 0, then the metadata value becomes 0 as well.
|
The last commits are mainly adding the specs. The specs are currently testing the "baseline", like throws-when-something-is-undefined, and the following cases:
These are all generated by creating an embedded glTF of a unit square with a property texture that contains all combinations of the RGBA bytes (0,127, 255) in the upper 9x9 pixels. There's still the issue that is mentioned in the previous comment, namely, that the actual metadata values, as obtained via the What also may have to be added is a spec for the |
EDIT: Much of what I originally wrote here may be obsolete. If it can be re-confirmed on another PC or in CI, I'll move it into a dedicated issue. |
I'm pretty confused right now, for two reasons:
|
These ❌ 'es are going to cause me nightmares 😬 Now there's a green check mark. Essentially, it was necessary to disable the tests in CI, due to WebGL not really being available. There is some documentation about the different "types and categories" of tests in the testing guide. But knowing how to configure these tests properly is a bit hard to derive from that. (I could say: There should be more documentation. But it would be even better if it was not necessary to add further documentation. Maybe it's not so much of an issue, because it "rarely" happens that a whole new spec file (or "category of tests") has to be added. And when it is done, it is usually done by someone who knows the details better than me...) |
@jjhembd When you have a chance to look at the latest state, here, that would be appreciated. The overall approach didn't really change since the last review pass. The main work since then consisted of
|
The 3D Tiles Next Metadata Compatibility Matrix currently lists the functionalities of picking and styling based on property textures or property attributes as "not supported". Some details about this requirement are also summarized in #9852
This PR is supposed to help fleshing out possible approaches for this. I have not worked with the relevant parts of the code. And the compatibility matrix contains a footnote saying
but I have no idea what this really entails. So I'm opening this as an early DRAFT, to get an idea about how flawed the approach is, and gather feedback for "better" approaches.
30-second-summary
The naive description of the approach is:
Scene.pickMetadata(schemaId, className, propertyName)
Implementation draft
There is a
Scene.pickMetadata
function. This function receives theschemaId/className/propertyName
of the metadata that is supposed to be picked.Note: This is only a DRAFT. It assumes that the client already knows what should be picked. But the information about what can be picked may have to be exposed in one form or another. Maybe via some
pickMetadataSchema
function that returns theMetadataSchema
of the picked object. Details TBD.The function directly delegates to a
Picking.pickMetadata
function. This roughly follows the pattern of the otherpick...
functions: It sets up everything to prepare a "metadata picking pass", renders everything into a frame buffer, and returns the rendered pixels at the picked position.Now, the crucial question is how the actual metadata property values end up in the frame buffer during this "metadata picking pass". And I know that the current solution is not right, but hope for feedback about better approaches. The current flow of events is:
Scene.pickMetadata
function has to callmodel.resetDrawCommands();
to enforce re-building the model draw commands for with the "picking shader". It would be better to solve this with somemodel.drawCommands[42].pickMetadata = true;
. But the structure of the model draw commands is... not obvious, and has many intricacies, and ... eventually, the shaders (or at least, parts of them) will have to be re-built, because the selection of the pickedclassName/propertyName
does happen at runtime after all.Picking.pickMetadata
function puts theclassName/propertyName
into theFrameState
. This is probably far too global and too transient. But this information is passed in from the client, and has to end up in a shader. I don't know whether there is a better "path" to put this information into the right place. (There are some additional quirks there, related to OIT, but... let's ignore that for now)ModelRuntimePrimitive
- namely, theMetadataPickingPipelineStage
. This will set the required#define
s in the shader, based on the presence of theclassName/propertyName
in theFrameState
.ModelFS.glsl
now checks for theMETADATA_PICKING
define. When it is defined, then it skips certain operations, and instead, runs themetadataPickingStage
. This obtains the "color" for the fragment from the metadata values (namely, the value that was selected via theMETADATA_PICKING_PROPERTY_NAME
define, that was set to be thepropertyName
). (Note: The fact that theModelFS.glsl
has to be broken up like that is not "nice", but this could and would be structured "more nicely", eventually)Open quesitons
There are some obvious things that have to be handled. For example: How is the actual property value encoded into the RGBA components? Right now, it is just written into the
R
channel, just for a test. How could, for example, aDOUBLE
metadata value be encoded? Do we need (or should be use) "floating point frame buffers"? And eventually, regardless of how the metadata value is encoded into the RGBA values, these values will have to be de-coded again in theScene.pickMetadata
function, to return the actual value to the user.However, these are things that can be addressed after a general, sensible approach was developed. And my gut feeling is: The approach that is drafted here is likely not such a "general sensible" approach. I'm open for suggestions.
Example
The following is a Sandcastle that uses the Custom Shaders Property Textures Sandcatle data set to show the current state of the picking. It calls
scene.pickMetadata
using the class/property names of that data set, and prints the returned values to the console:Again, these are just returned as 4-element byte arrays with the "value" encoded in the first byte, but details about the encoding/decoding can be decided later.