3d Tiles composite padding breaks instanced tiles #5612
Conversation
… required padding would have a bad URL because the padding would end up appended to the url.
|
|
||
| // We need to remove padding from the end of the model URL in case this tile was part of a composite tile | ||
| // This removes all white space and null characters from the end of the string. | ||
| gltfUrl = gltfUrl.trim().replace(/\0+$/, ''); |
There was a problem hiding this comment.
Can't this just be gltfUrl.replace(/\s+$/, ''); Why the need for trim or null?
There was a problem hiding this comment.
And shouldn't we add a unit test for this?
There was a problem hiding this comment.
Because trim and \s only match whitespace characters which according to the docs are:
A space character
A tab character
A carriage return character
A new line character
A vertical tab character
A form feed character
null would be a common pad character as well as a lot of languages initialize buffers to 0.
There was a problem hiding this comment.
OK, but if there's whitespace followed by null followed by more whitespace, this will fail right? Should you get rid of trim and have a regex that removes all null and whitespace at the end?
|
Tests and tweaks to regex should be done @mramato |
|
Thanks, @lilleyse please review the new tests and merge if you're happy. Thanks. |
|
Update CHANGES.md please. |
|
Should the test use cmpt tiles instead of i3dm? Also sorry to add extra work for this, but the other tilesets in the specs folder are created in the samples-generator project and it would be good to create a new one there. |
|
It probably won't be till next week when I can get a chance to regenerate the sample using |
|
@lilleyse Added sample to samples-generator (CesiumGS/3d-tiles-validator#111). Should be good to go now. |
|
Updated the files from CesiumGS/3d-tiles-validator#111. All else looks good. |
Fixed issue where a composite tile containing and instanced tile that required padding would have a bad URL because the padding would end up appended to the url. There is probably a better way to handle this at the spec level.
CC @lilleyse