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

3d Tiles Inspector #4872

Merged
merged 42 commits into from
Feb 7, 2017
Merged

3d Tiles Inspector #4872

merged 42 commits into from
Feb 7, 2017

Conversation

austinEng
Copy link
Contributor

@austinEng austinEng commented Jan 17, 2017

Progress toward #4846. I would love to have some initial feedback on this

This replaces the debug options in the Sandcastle example http://localhost:8080/Apps/Sandcastle/index.html?src=3D%20Tiles.html&label=Showcases with an inspector widget. All previous functionality has been ported with the exception of the styling options.

Additional Todos:

  • Show URL of clicked tile
  • Click on tileset to inspect
  • Styling integration
  • Port other Sandcastle examples

Example initialization:

viewer.extend(Cesium.viewerCesium3DTilesInspectorMixin);
var model = viewer.cesium3DTilesInspector.viewModel;

model.tileset = new Cesium.Cesium3DTileset({
    url: ...
});

Preview:
inspector

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 17, 2017

Styling integration?

Perhaps we could do a super simple style editor that shows the current style source and allows users to change it with automatic recompile.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 17, 2017

@lilleyse can you please review the code? I am only going to look at the functionality. I'll do a quick glance at the graphics-related code at the end.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 17, 2017

The Inspector should not include the dropdown of tilesets - that is specific to the Sandcastle example or someone's app.

Instead, leave the dropdown as part of the Sandcastle example and programmatic change the Inspector's focus when a new tileset is loaded. We also want the ability for someone to pick on a tileset and open the Inspector (this might just be example code to do a pick then create the Inspector).

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 17, 2017

Perhaps remove "Picking" when "Enable Picking" is not checked.

image

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 17, 2017

Perhaps remove "Picking" when "Enable Picking" is not checked.

Really, these are Sandcastle-specific features that I just added at some point for testing. I don't think they need to be part of the inspector at all. Will they help with debugging and profiling? Maybe hide feature would, but I don't know that it is important enough.

These are probably better served as part of the Sandcastle example, perhaps just cleaned up compared to the original code.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 17, 2017

@lilleyse when the "Request Volume" tileset is selected and "Request Volume" is checked, the volume disappears when the tile disappears. Is this because of the SSE? Or because the request volume failed? If it's the later, should we still draw the request volume but perhaps another color?

bv

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 17, 2017

Include a text box so we can explicitly set the SSE.

The slider range should probably be wider, maybe 0 to 128. Just a heads up that zero will load the entire tileset.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 17, 2017

Can we make the stats easier to read? Perhaps one per line with bullets and make the text (not numbers) bold? Maybe look at Renderdoc for inspiration.

Also checking both "Stats" and "Pick Stats" hides the details of the "[Color]" pass:

image

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 17, 2017

Great start, just those comments.

@lilleyse
Copy link
Contributor

@lilleyse when the "Request Volume" tileset is selected and "Request Volume" is checked, the volume disappears when the tile disappears. Is this because of the SSE? Or because the request volume failed? If it's the later, should we still draw the request volume but perhaps another color?

It's the latter in this case - though the former could be true too. If the tile is not reached for any reason in selectTiles it won't add its debug command.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 17, 2017

It's the latter in this case - though the former could be true too. If the tile is not reached for any reason in selectTiles it won't add its debug command.

Do you think it is worth changing so that the debug volume can be shown? I don't want to complicate the render loop or confuse the debug view (which is why I said to change the color).

@lilleyse
Copy link
Contributor

Do you think it is worth changing so that the debug volume can be shown? I don't want to complicate the render loop or confuse the debug view (which is why I said to change the color)

It could be doable by keeping track of which tiles have request volumes. I'll think about how to cleanly add this.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 17, 2017

I'll think about how to cleanly add this.

OK, but it is fine to bail if the tile selection code gets messy.

@austinEng austinEng force-pushed the 3d-tiles-inspector branch 2 times, most recently from f17903d to 2358ba7 Compare January 18, 2017 20:36
@austinEng austinEng force-pushed the 3d-tiles-inspector branch 4 times, most recently from 69ee7f9 to 2efeea6 Compare January 19, 2017 15:01
Copy link
Contributor

@lilleyse lilleyse left a comment

Choose a reason for hiding this comment

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

The code changes are good. I just have a few more suggestions after using the inspector some more:

  • Sometimes the picking coloring doesn't coordinate well with applying styles / colorize. For example, edit a style to use a different color (don't compile), hover over a building, press ctrl+enter, and move mouse off the building - it still uses the saved old color.
  • It would be nice for the style to be reapplied when clicking compile even if the text hasn't changed. Example: turn colorize on/off, then press compile style button - style should be reapplied even though it hasn't changed.
  • Is it possible to support the tab key in the editor instead of switching focus?

if (val) {
that._eventHandler.setInputAction(function(e) {
that._feature = scene.pick(e.endPosition);
that._updateStats(true, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

This one still passes 2 arguments.

@austinEng
Copy link
Contributor Author

It would be nice for the style to be reapplied when clicking compile even if the text hasn't changed. Example: turn colorize on/off, then press compile style button - style should be reapplied even though it hasn't changed.

In addition to adding a force re-compile I think it would make sense to also reapply styles when colorize is disabled

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 1, 2017

@austinEng I just noticed 11d89a6. Is this ready?

@austinEng
Copy link
Contributor Author

@pjcozzi Not quite
@lilleyse Not certain how to forcefully reapply the styles after disabling debugColorizeTiles. Seems like marking the styleEngine as dirty doesn't work because the frameState is a render pass. I could probably loop though all the features and reapply the styles one-by-one, but that seems messy.

@austinEng
Copy link
Contributor Author

Not certain how to forcefully reapply the styles after disabling debugColorizeTiles. Seems like marking the styleEngine as dirty doesn't work because the frameState is a render pass. I could probably loop though all the features and reapply the styles one-by-one, but that seems messy.

Okay, I figured this out. debug flags in the tiles aren't updated immediately upon changing flags of the tileset so things were getting reset.

I'm getting jsHint errors in Specs/Renderer/TextureSpec.js which I haven't modified. I'm guessing this happened after merging in 3d-tiles?

@lilleyse
Copy link
Contributor

lilleyse commented Feb 1, 2017

I'm getting jsHint errors in Specs/Renderer/TextureSpec.js which I haven't modified. I'm guessing this happened after merging in 3d-tiles?

Yeah don't worry about those. Once the texture compression PR is merged back into 3d-tiles they should be gone.

@lilleyse
Copy link
Contributor

lilleyse commented Feb 3, 2017

It's probably worth addressing #4932 (comment) before this PR is merged.

@austinEng
Copy link
Contributor Author

@lilleyse Ported the debug camera for freeze frame and fixed the on-click re-styling issue. The problem is that the Sandcastle example was setting the property 'clicked' to true/false which ends up marking all feature properties as dirty and causing a re-style.

I didn't really see the purpose of this 'clicked' property because all the example did was console.log if that feature had already been clicked so I removed it. Is this property needed elsewhere?

@lilleyse
Copy link
Contributor

lilleyse commented Feb 3, 2017

The clicked setting is just example code for creating a property dynamically and then getting it. It isn't needed elsewhere. I don't really mind if we keep it or not.

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 7, 2017

What is the plan here? Can we merge this before the bug bash on Wednesday?

@austinEng
Copy link
Contributor Author

I think this is finished, but the tests no longer pass after merging with 3d-tiles. However, I think @lilleyse said this was a problem elsewhere

@lilleyse
Copy link
Contributor

lilleyse commented Feb 7, 2017

All looks good to me - @pjcozzi would you like review as well?

@pjcozzi pjcozzi merged commit cf5ece2 into CesiumGS:3d-tiles Feb 7, 2017
@skyrs
Copy link

skyrs commented Feb 8, 2017

image
I use 3d-tiles load my 3d model , but the result is very bad, so Who know why? I need set some property? my data is from osgb to b3dm , contain Pagedlod

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 8, 2017

@skyrs Could be the lighting or materials.

Please ask questions like this on the Cesium forum. For more info on where to post, see here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants