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

Remove 3D Tiles deprecation warnings #5363

Closed
lilleyse opened this issue May 24, 2017 · 18 comments
Closed

Remove 3D Tiles deprecation warnings #5363

lilleyse opened this issue May 24, 2017 · 18 comments
Labels

Comments

@lilleyse
Copy link
Contributor

Batched3DModel3DTileContent has special handling for legacy b3dm headers. This should be removed once we're pretty sure most tilesets have upgraded.

@pjcozzi
Copy link
Contributor

pjcozzi commented May 24, 2017

Labeled with the new category - 3d tiles label. Please use this moving forward on all 3D Tiles issues. I haven't yet went back through them.

@lilleyse
Copy link
Contributor Author

lilleyse commented Jun 9, 2017

Also at the same time remove support for the non-underscored BATCHID semantic.

@lilleyse lilleyse mentioned this issue Jun 9, 2017
23 tasks
@lilleyse
Copy link
Contributor Author

And drop support for lowercase 'add' and 'refine'.

@lilleyse
Copy link
Contributor Author

And turn the geometric error check back into a RuntimeError : #5574

@lilleyse lilleyse changed the title Remove legacy b3dm headers Remove 3D Tiles deprecation warnings Jun 30, 2017
@mramato
Copy link
Contributor

mramato commented Jan 18, 2018

And remove the workaround added in #6125

@lilleyse
Copy link
Contributor Author

Remove workaround that allows loading unaligned glb's. The alignment rules are much more solid in the 1.0 spec.

When fixing, look for The embedded glb is not aligned to a 4-byte boundary

@lilleyse
Copy link
Contributor Author

lilleyse commented Jul 3, 2018

Throw an error if a tile's content is glTF 1.0.

@lilleyse
Copy link
Contributor Author

lilleyse commented Jul 3, 2018

Does anyone have thoughts on when to start removing the workarounds for these deprecated 3D Tiles syntax/features?

I was thinking the best time would be

  • After the 1.0 branch of the spec is merged into master
  • Finish the upgrade command in 3d-tiles-tools so that people can upgrade their old tilesets
  • Ideally, finish the 3D Tiles validator, if there is time.

Thoughts @pjcozzi, @mramato, @ggetz ?

@mramato
Copy link
Contributor

mramato commented Jul 3, 2018

I'm all for removing as many workarounds/non-spec compliant behavior as possible.

Throw an error if a tile's content is glTF 1.0.

Except this one, is there a reason to do this? It will break almost every tileset we currently have. What is the burden on just supporting gltf 1.0 forever? I thought we automatically updated models to 2.0 anyway so I would think the burden is small. (but I could be wrong)

@lilleyse
Copy link
Contributor Author

lilleyse commented Jul 3, 2018

There isn't any burden in supporting gltf 1.0, but it's technically not valid to be in 3D Tiles 1.0 and I wasn't sure how hard-line we wanted to be.

We have another large breaking change where tile.content.url is now tile.content.uri #6744. If the goal is to not break most existing tilesets then we should also keep the workaround that can load either variant.

With a few exceptions, a lot of the workarounds are actually pretty simple with little code burden. We could just keep most of them around forever if we wanted.

@ggetz
Copy link
Contributor

ggetz commented Jul 3, 2018

After the 1.0 branch of the spec is merged into master

When is the plan to do this? I was under the impression that master was the "working" branch and "1.0" is basically the tagged release version for the community standard.

@mramato
Copy link
Contributor

mramato commented Jul 3, 2018

With a few exceptions, a lot of the workarounds are actually pretty simple with little code burden. We could just keep most of them around forever if we wanted.

In that case, my vote would be that anything that's less than a few lines and doesn't affect performance should stick around forever and just log warnings. We should also make sure there is a clear comment stating that it is there for backwards compatibility reasons. More complicated workarounds (like #6125) should definitely get removed. Ultimately, my thinking is that we shouldn't shift the burden to our users (by making them update all of their tilesets) unless there is a good reason to.

But this is just my opinion and I'm sure others may feel differently.

@lilleyse
Copy link
Contributor Author

lilleyse commented Jul 3, 2018

I'm on board with that. And #6125 is a perfect example of one of the exceptions that should be removed.

@lilleyse
Copy link
Contributor Author

lilleyse commented Jul 3, 2018

When is the plan to do this? I was under the impression that master was the "working" branch and "1.0" is basically the tagged release version for the community standard.

I think the two things that we need are:

I could see this happening alongside the 1.48 release.

@ggetz
Copy link
Contributor

ggetz commented Jul 5, 2018

We added a depreaction warning for content.url instead of content.uri: #6744 (comment)

@lilleyse
Copy link
Contributor Author

lilleyse commented Jul 5, 2018

Given the content.url deprecation warning, this is also a good excuse to update all the hosted tilesets to 3D Tiles 1.0, glTF 2.0, and Draco for #6487.

Currently working on the upgrade command in 3d-tiles-tools that will help upgrading some of these. It will also fix the "The embedded glb is not aligned to a 4-byte boundary." warning in the power plant model.

@ggetz
Copy link
Contributor

ggetz commented Jul 13, 2018

Added a deprecation warning fro the Batch Table Hierarchy in #6780

@lilleyse
Copy link
Contributor Author

Closing - we're keeping most workarounds for backwards compatibility so no action needed here.

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

No branches or pull requests

4 participants