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

Fix replacement refinement when external tileset is loaded #3517

Merged
merged 8 commits into from
Jun 15, 2016

Conversation

lilleyse
Copy link
Contributor

@lilleyse lilleyse commented Feb 4, 2016

For #3241

Addressing the issue in #3456 (comment) where replacement refinement draws empty space when loading an external tileset.

@e-andersson Thanks for bringing this up, it's an edge case that I forgot to handle. I added a test for good measure. Let me know if this fixes the problem for you.

@e-andersson
Copy link
Contributor

It fixes the blank, but when zooming in further it looks like more than one layer is being selected. Is the tile above the tileset content tile possibly being selected over and over again and not being replaced?

@lilleyse
Copy link
Contributor Author

lilleyse commented Feb 4, 2016

I'm not seeing that problem with my testing. The TilesetReplacement3 data set seems to work okay in the 3D Tiles Sandcastle. Is your tileset arranged similarly?

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 4, 2016

The code change is OK with me. @e-andersson let us know when this satisfies your needs.

@e-andersson
Copy link
Contributor

I think it might be that I'm missing the "refine": "add" for the tileset content tile, compared with the TilesetReplacement3 set. Had forgotten about this, I think we discussed it last year. So do tilesets have to specify "refine": "add" for tileset content tiles?

Anyway, I'll tinker with the metadata and get back when I have the results.

@e-andersson
Copy link
Contributor

The issue remains after setting "refine": "add". My tileset has a slightly different structure, and though it doesn't necessarily have to be structured this way I don't see why it shouldn't work.

A - b3dm content tile
B - tileset.json tile

geometricError: 128     A      A  <-- these get selected together with descendants
geometricError: 64      B      B
geometricError: 64      A      A    <-- TilesetReplacement3 data has empty tiles here
geometricError: 32    A   A  A   A

With this structure, it seems that the tiles above tileset.json tiles will be selected even when they should be replaced by tiles further down. It looks as though these tiles were set to additive refinement although they aren't.

@lilleyse It seems the test datasets uses an empty tile as the root tile in the sub tree and then branches out. Does this somehow affect selection?

@lilleyse
Copy link
Contributor Author

lilleyse commented Feb 5, 2016

So do tilesets have to specify "refine": "add" for tileset content tiles?

It should work the same with "replace" or "add" since it skips that step.

I created a tileset based on your example and I'm seeing the same problems. I'm looking into it now.

@lilleyse
Copy link
Contributor Author

lilleyse commented Feb 5, 2016

Hopefully this change fixes it for you. The problem was the the upper tile would only refine if its descendants were selected, but didn't account for if they too were replaced.

@e-andersson
Copy link
Contributor

@lilleyse Sorry, but there are still cases where it doesn't work. If I zoom in quickly, so that each LOD does not have time to finish loading, I'll sometimes end up with double layers of tiles, one completely refined layer and one rougher layer like before, that shouldn't be selected.

If I then move the camera around for a while, the wrong tiles seem to finally get deselected.

@e-andersson
Copy link
Contributor

I think in this data set there are around 13 or so layers of tiles, with one layer of tileset.json tiles somewhere in the middle. Does the request ordering cause trouble with larger number of requests?

@lilleyse
Copy link
Contributor Author

lilleyse commented Feb 8, 2016

I'll be looking into this more today. Does the tileset contain empty tiles, or is it all full with the exception of the external tileset.json's?

@e-andersson
Copy link
Contributor

It is all full with the exception of external tilesets, if I remember
correctly. Can't verify at the moment but I can have a look tomorrow when
I'm back at work.
On 8 Feb 2016 3:00 p.m., "Sean Lilley" notifications@github.com wrote:

I'll be looking into this more today. Does the tileset contain empty
tiles, or is it all full with the exception of the external tileset.json's?


Reply to this email directly or view it on GitHub
#3517 (comment)
.

@e-andersson
Copy link
Contributor

Just to confirm, the tileset does not contain empty tiles.

@lilleyse
Copy link
Contributor Author

lilleyse commented Feb 9, 2016

Can you try it out from 321cf9b? I want to know if #3456 introduced the problems.

@e-andersson
Copy link
Contributor

The problem is in that commit as well. I went back to 9cf62aa and checked too, just to get a point some way before that commit, but it was the same. I think I won't be able to check further back than that with the current tileset.

@e-andersson
Copy link
Contributor

@lilleyse any progress on this? Do you need any additional information?

@lilleyse
Copy link
Contributor Author

Sorry about the inactivity lately, I've been busy with other work.

I'm trying to figure out if this problem is inherent to selectTiles or if it was introduced after we started handling empty space issues. If you comment out the checkRefiningTiles line, does the problem still occur? (Ignoring the blank spaces that may appear). Do you notice the same problem with data sets that don't have external tilesets?

One thing we need to do on our end is create better test data sets for replacement refinement. Once I have this I will be able to debug this a lot easier.

@e-andersson
Copy link
Contributor

No hurry, we're not suffering from it at the moment.

I tried commenting out the checkRefiningTiles line but I'm not quite sure what you mean when you say "Ignoring the blank spaces that may appear" since that problem is pretty much just that.

@e-andersson
Copy link
Contributor

@lilleyse any progress on this?

@lilleyse
Copy link
Contributor Author

I haven't been working in 3d-tiles lately but I'll try to look into this more today or tomorrow.

@e-andersson
Copy link
Contributor

Great, thanks!

On Thu, Mar 31, 2016 at 3:42 PM, Sean Lilley notifications@github.com
wrote:

I haven't been working in 3d-tiles lately but I'll try to look into this
more today or tomorrow.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#3517 (comment)

Erik Andersson

Mobile +46 704244916

Email erik.andersson@vricon.com

Vricon Systems AB

SE-581 88 Linköping, SWEDEN

www.vricon.com

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 14, 2016

I can reproduce this, see #3424 (comment).

@lilleyse
Copy link
Contributor Author

Sorry about the huge delay with this, but I think I fixed this problem. When a tile checked if it could refine, it didn't consider if any of its descendants-with-content were out of view - in which case it should still be able to refine.

I'm still noticing a few other artifacts, but they don't seem to be related to this as they also appear with a non split up tileset.json.

@e-andersson let me know if this works for you, and thanks for the patience.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 15, 2016

Code and tests LGTM.

@e-andersson we deeply appreciate your testing here if you have the time.

@e-andersson
Copy link
Contributor

@lilleyse @pjcozzi Great, ran a quick test and it looks good now! Good job :)

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 15, 2016

Very nice, thanks @lilleyse @e-andersson!

@lilleyse please keep #3241 up-to-date.

@pjcozzi pjcozzi merged commit 5f0e1dc into 3d-tiles Jun 15, 2016
@pjcozzi pjcozzi deleted the 3d-tiles-refinement-fix branch June 15, 2016 17:12
@lilleyse lilleyse mentioned this pull request Jul 13, 2016
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.

None yet

3 participants