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

Deprecate loadCRN and loadKTX #9478

Merged
merged 5 commits into from
Apr 9, 2021
Merged

Deprecate loadCRN and loadKTX #9478

merged 5 commits into from
Apr 9, 2021

Conversation

ebogo1
Copy link
Contributor

@ebogo1 ebogo1 commented Apr 9, 2021

This deprecates loadCRN and loadKTX as we will move to support KTX2 in Cesium 1.82.

@cesium-concierge
Copy link

Thanks for the pull request @ebogo1!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ 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.
  • Works (or fails gracefully) in IE11.

@ebogo1 ebogo1 requested a review from lilleyse April 9, 2021 17:40
CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
Source/Core/loadCRN.js Outdated Show resolved Hide resolved
Source/Core/loadKTX.js Outdated Show resolved Hide resolved
@lilleyse
Copy link
Contributor

lilleyse commented Apr 9, 2021

Just noting here that once we remove loadCRN and loadKTX we will need to

  • Update the Image-Based Lighting sandcastle
  • Update the Materials sandcastle

Both of these use KTX textures

@ebogo1 ebogo1 mentioned this pull request Apr 9, 2021
6 tasks
@ebogo1
Copy link
Contributor Author

ebogo1 commented Apr 9, 2021

@lilleyse I updated the deprecation warning wordings in 37130dd.

Just noting here that once we remove loadCRN and loadKTX we will need to

I also opened #9479 to keep track of the KTX2 to-do list.

@lilleyse
Copy link
Contributor

lilleyse commented Apr 9, 2021

I pushed some tweaks to CHANGES.md above. Otherwise looks good. Merging since CI passed on the last code changes, my changes were just CHANGES.md.

@lilleyse lilleyse merged commit 3bca4a0 into master Apr 9, 2021
@lilleyse lilleyse deleted the deprecate-ktx branch April 9, 2021 22:05
@mramato
Copy link
Contributor

mramato commented May 3, 2021

@lilleyse @ebogo1 Why did we deprecate loadKtx but keep it's Sandcastle example? If something is deprecated it shouldn't be used anywhere in the examples.

@mramato
Copy link
Contributor

mramato commented May 3, 2021

It's in both the Materials and Image Based Lighting demos.

@ebogo1
Copy link
Contributor Author

ebogo1 commented May 3, 2021

One option is to roll back the deprecation warnings and add them as part of the KTX2 PR. If delaying that PR by adding back loadKTX/CRN isn't worth it, maybe we can just remove the "compressed image" option from Materials and use a non-ktx file in Image-Based Lighting for now?

Both Sandcastles are using KTX 2.0 in the new PR.

@mramato
Copy link
Contributor

mramato commented May 3, 2021

I think the main reason we wanted to deprecate was because no one as far as we know is actually using old KTX and it was easier to make a clean break. If the Sandcastle examples are just updated to use KTX2 in your PR, then it would be silly to remove them and then re-add them..

Both Sandcastles are specific to compressed textures, I believe, so we can't really make them use something else (and it's probably not worth it).

@mramato
Copy link
Contributor

mramato commented May 3, 2021

Okay, since @lilleyse is MIA, I'm going to just proceed as things currently are. We want to drop CRN/KTX asap so getting the notice in now is probably the best choice.

@lilleyse
Copy link
Contributor

lilleyse commented May 3, 2021

I think it's fine to proceed. There's no way to remove KTX1 support from the Image-Based Lighting sandcastle without breaking it before KTX2 is in.

The other approach could have been to add KTX2 and in the same PR have an immediate breaking change for CRN/KTX1, but that doesn't give as much warning.

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