Cesium 3dtiles inspector refactor #5217
Conversation
The problem isn't the amount of code that changed or GitHub, it's that Git did not pick up the rename automatically (which is a weakness of Git in general). In either case, I can code review it as if it's new since you changed a bunch of stuff and I haven't looked to closely at this yet anyway. Any other testing @austinEng or any of the other 3D Tiles team can provide is greatly appreciated. |
| set : function(value) { | ||
| dynamicSSEDensity(value); | ||
| if (that._tileset) { | ||
| that._tileset.dynamicScreenSpaceErrorDensity = Math.pow(value, 6); |
There was a problem hiding this comment.
This is the only change I wasn't 100% sure on. This value was previously being set in the view
Is there any significance to the exponent of 6?
There was a problem hiding this comment.
I believe I just copied whatever the default value was for a tileset, which is also somewhat arbitrary
There was a problem hiding this comment.
This was to make an exponential slider since sse values tend to be a lot closer to 0 than 1. This code looks fine, may just need a comment.
|
I tested a little bit and it seems to work like normal. |
| * @type {Number} | ||
| * @default 16 | ||
| */ | ||
| this.maximumSSE = 16; |
There was a problem hiding this comment.
Any public methods and properties should spell out ScreenSpaceError instead of abbreviating to SSE. (I know you it was already like this, but this is a refactor of something that isn't even in master yet so we should fix it).
| Cesium3DTilesInspectorViewModel.prototype.compileStyle = function() { | ||
| if (defined(this._style)) { | ||
| if (this.styleString !== JSON.stringify(this._style.style)) { | ||
| if (defined(this._tileset)) { |
There was a problem hiding this comment.
Invert and combine some of these statements to avoid 3 levels of if nesting. A bunch of this properties are accessed multiple times in this function, break them out into locals.
| get : function() { | ||
| return this._tileset; | ||
| }, | ||
|
|
There was a problem hiding this comment.
Whitespace (also run the auto-formatter on this file).
| that._properties(t.properties); | ||
| }); | ||
|
|
||
|
|
| var that = this; | ||
| tileset.readyPromise.then(function(t) { | ||
| that._properties(t.properties); | ||
| }); |
There was a problem hiding this comment.
Shouldn't this handle failure somehow? What actually happens if ready is never fired or fails?
| if (defined(tileset)) { | ||
| var that = this; | ||
| tileset.readyPromise.then(function(t) { | ||
| that._properties(t.properties); |
There was a problem hiding this comment.
This is a race condition. If the viewModel is destroyed before the tileset is ready, this will throw. Check that.isDestroyed() first.
| * Handles key press events on the style editor | ||
| */ | ||
| Cesium3DTilesInspectorViewModel.prototype.styleEditorKeyPress = function(sender, event) { | ||
| if (event.keyCode === 9) { //tab |
There was a problem hiding this comment.
This block is crying for a real editor, like CodeMirror, but I guess there's nothing stopping someone from putting CodeMirror on top of this view model and ignoring this function.
|
Great work here @hpinkos! Thanks for cleaning this up. That's all I have. |
|
@mramato ready |
|
Thanks again @hpinkos, good work here. |
The knockout usage in
Cesium3dTilesInspectorwas not correct. There was an incorrect separation of the view and viewmodel. I also cleaned up the code because there were a ton of unnecessary subscriptions and bad use of private properties. No functionality changed, but I apparently changed so much of the code that GitHub can't do a proper diff.@austinEng can you test to make sure all of the functionality is there?
@mramato do you want to do a code review?