Allow passing tileset options to reality data #12801
Conversation
|
Thank you for the pull request, @javagl! ✅ We can confirm we have a CLA on file for you. |
|
That formatting commit was a strange one. The |
| // The maximum screen space error was defined to default to 4 for | ||
| // reality data tilesets. This will only be overridden if it was | ||
| // specified in the tilesetOptions explicitly. |
There was a problem hiding this comment.
I think this comment can be more descriptive other than "it was like this before this change"
We set this value because we noticed the reality data tilesets were commonly not loading enough detail when zoomed out compared to what is expected. A comment that explains that context would be useful here.
The second half is also a bit redundant. I feel the spread ... operator and behavior should be enough to explain the behavior here (but that's bikeshedding)
@ggetz I believe there was some discussion around fixing this on the data side within the itwin platform. do you know if that ever got addressed?
There was a problem hiding this comment.
This PR is now assigned to you - was that intentional?
If not, I can update the comment, and try to be more clear.
Regardless of the spread operator (and even though I actually tried out whether the difference of
x = { ...y, value: 123 }; and
x = { value: 123, ...y };
is the one that one could expect (I've learned to not ever expect anything in JavaScript to be "sane" in any way 😆 ), I think it's important to make clear that somewhere, someone, decided that 4 should be the default here. If there was any reasoning behind that (beyond "We tried it, and it ~'worked better'"), then I could add this as well.
Maybe all these values are "wrong", though. See #7715 and, more generally #4043 ...
There was a problem hiding this comment.
This PR is now assigned to you - was that intentional?
I just thought it might be easier to track that I would be doing the review, maybe that's the wrong process. Please continue making changes
Regardless of the spread operator...
Objects defined with duplicate keys, especially common when using the spread operator, take the value of the "later" one, or the one lower in the code. So { ...options, value: 'a' } defaults to the spread values and uses the overrides for value. { value: 'a', ...options } would default to the value but use the value from the options if it's defined.
I think it's important to make clear that somewhere, someone, decided that 4 should be the default here.
Regardless I do agree we should explain why which is due to the source data tending to be lower "resolution" than many other tilesets at the default sse
There was a problem hiding this comment.
I would have expected the values to be assigned in the order in which they are listed or fetched from ...spreaded objects (including overriding of the previous ones). I still tried it out. (Yes, it's specified in https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_syntax#overriding_properties , but I still tried it out 😆 )
I now extended the comment a bit, saying that the usual default (16) was just not enough. Whether changing the default value is preferable compared to just telling iTwin to do a blanket geometricError *= 4? I don't know.
There was a problem hiding this comment.
Whether changing the default value is preferable compared to just telling iTwin to do a blanket geometricError *= 4? I don't know.
This is probably part of a larger discussion but it shouldn't hold up this PR. Happy to merge it and change later if the data changes
CC @ggetz
Description
Allow passing a
tilesetOptionsobject toITwinData.createTilesetForRealityDataId. The tileset options will be forwarded to the tileset that is created internally. One caveat is that themaximumScreenSpaceErrorfor reality data tilesets was set to 4 by default. This should still be the default, with the option of overriding that default with the given options.Issue number and link
#12709
Testing plan
Basic specs have been added for passing on the options, including the special handling of the
maximumScreenSpaceError.Author checklist
CONTRIBUTORS.mdCHANGES.mdwith a short summary of my change