cesium icon indicating copy to clipboard operation
cesium copied to clipboard

Cesium3DTileset and Model warnings for debugWireframe and enableDebugWireframe misuse

Open Sn00pyW00dst0ck opened this issue 3 years ago • 4 comments

Changes

Implemented changes to constructor functions and property setters for Model and Cesium3DTileset to provide the user with warnings and information about possible misuse of the enableDebugWireframe and debugWireframe flags.

Implemented changes to the Cesium3DTilesInspector to disable the wireframes checkbox when enableDebugWireframe is set to false and add text explaining why the box has been disabled. This change added a knockout property to the Cesium3DTilesInspectorViewModel, called hasEnabledWireframe, which determines whether enableDebugWireframe is set to true or false and determines if the checkbox should be enabled or not.

Test Steps

Tested all changes extensively within the sandbox environments and found all implemented warnings & UI changes work as expected. These changes also passed all tests that the repository was able to pass when I cloned it.

Issue Link

Issue: #10608

Sn00pyW00dst0ck avatar Sep 21 '22 01:09 Sn00pyW00dst0ck

Thanks for the pull request @Sn00pyW00dst0ck!

  • :heavy_check_mark: Signed CLA found.
  • :grey_question: CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • :grey_question: Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • [ ] Cesium Viewer works.
  • [ ] Works in 2D/CV.

cesium-concierge avatar Sep 21 '22 01:09 cesium-concierge

Marking this as a draft temporarily so I can properly setup Travis Integration. I thought I'd already done so but apparently not 🤷.

Sn00pyW00dst0ck avatar Sep 21 '22 01:09 Sn00pyW00dst0ck

Thanks @Sn00pyW00dst0ck. I recently tweaked a setting that was introduced in https://github.com/CesiumGS/cesium/pull/10780. You should be able to have travis run without having to setup any additional integration.

@ptrgags Would you be able to take a look at this PR once ready?

ggetz avatar Sep 21 '22 13:09 ggetz

Thanks @Sn00pyW00dst0ck. I recently tweaked a setting that was introduced in #10780. You should be able to have travis run without having to setup any additional integration.

@ptrgags Would you be able to take a look at this PR once ready?

Awesome! If nothing is needed on my end to get Travis running then I'll mark this PR ready for review.

Sn00pyW00dst0ck avatar Sep 21 '22 13:09 Sn00pyW00dst0ck

This should be ready for merging now, but can I get someone to give this another review just in case anything else should be added / is missing? Thanks!

Sn00pyW00dst0ck avatar Sep 22 '22 15:09 Sn00pyW00dst0ck