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

selecting terrain brush on wrong layer can have massive consequences #1632

Open
Bobjt opened this Issue Jun 28, 2017 · 7 comments

Comments

Projects
None yet
3 participants
@Bobjt

Bobjt commented Jun 28, 2017

Tiled doesn't freeze but hangs the main thread for quite some time if I select the wrong layer in conjunction with the terrains brush. Stack trace attached (though it may not be needed).
Uploading Spindump-062817-terrain-brush.txt…

Steps to reproduce:

  • create a 300 x 300 map (or, create a new layer in an existing map)
  • select the terrain brush + a terrain and move mouse over the empty layer
  • witness hang for about a minute

The hang is avoided if I first fill the new layer with a tile that exists in the selected terrain. I didn't search for existing issues, so apologies if this is a duplicate report.

@Bobjt

This comment has been minimized.

Show comment
Hide comment
@Bobjt

Bobjt Jun 29, 2017

Re-uploading and including a spindump from a permanent lock.

Spindump-062817-terrain-lock-this-one-is-a-permanent-hang.txt

Spindump-062817-terrain-brush.txt

I'll try to get a repro' case posted here.

Bobjt commented Jun 29, 2017

Re-uploading and including a spindump from a permanent lock.

Spindump-062817-terrain-lock-this-one-is-a-permanent-hang.txt

Spindump-062817-terrain-brush.txt

I'll try to get a repro' case posted here.

@Bobjt

This comment has been minimized.

Show comment
Hide comment
@Bobjt

Bobjt Jun 29, 2017

It looks like the terrain logic is doing some weird stuff.

Here's a capture of the eventual preview from a long hang:
screen shot 2017-06-28 at 11 45 50 pm 2

And the map without the terrain tool preview looks like this:
screen shot 2017-06-29 at 12 13 59 pm

It was still hung however after this preview was rendered (maybe interactively hanging for long periods, if that makes any sense)

Bobjt commented Jun 29, 2017

It looks like the terrain logic is doing some weird stuff.

Here's a capture of the eventual preview from a long hang:
screen shot 2017-06-28 at 11 45 50 pm 2

And the map without the terrain tool preview looks like this:
screen shot 2017-06-29 at 12 13 59 pm

It was still hung however after this preview was rendered (maybe interactively hanging for long periods, if that makes any sense)

@Bobjt

This comment has been minimized.

Show comment
Hide comment
@Bobjt

Bobjt Jun 29, 2017

Steps to reproduce:

  1. open the attached overworld-draft-200.tmx
  • the external tileset it depends on is in the "_tilesets.zip" bundle - it's the addlsheet1.tsx
  • the graphics that addlsheet1.tsx depends on is attached. Unzip: addlsheet1.png.zip
  1. in Tiled, select the layer "Layer 0 a" (this is the layer on which all tiles are currently defined. All tiles on this layer are member of the various terrain definitions (even the clear portions). You'll see those after opening the terrains window on this map.

  2. select the terrain definition "oworld1 mountains" and bring the cursor over to the center of the map.

  3. experience the hang. That's all.

addlsheet1.png.zip
overworld-draft-200.tmx.zip
_tilesets.zip

Bobjt commented Jun 29, 2017

Steps to reproduce:

  1. open the attached overworld-draft-200.tmx
  • the external tileset it depends on is in the "_tilesets.zip" bundle - it's the addlsheet1.tsx
  • the graphics that addlsheet1.tsx depends on is attached. Unzip: addlsheet1.png.zip
  1. in Tiled, select the layer "Layer 0 a" (this is the layer on which all tiles are currently defined. All tiles on this layer are member of the various terrain definitions (even the clear portions). You'll see those after opening the terrains window on this map.

  2. select the terrain definition "oworld1 mountains" and bring the cursor over to the center of the map.

  3. experience the hang. That's all.

addlsheet1.png.zip
overworld-draft-200.tmx.zip
_tilesets.zip

@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Jul 4, 2017

Owner

So, this is due to a combination of factors:

  • Your "oworld1 mountains" terrain only has transitions to "nothing". In your tileset there does not exist any way to connect "oworld1 mountains" to the terrains used by the existing tiles on the "Layer 0 a". This causes it to change the whole map as it futilely tries to fix up the border, which keeps expanding to fill the whole map (which is relatively large). Using "oworld1 mountains" on an empty layer works fine, because the border is immediately fine.

  • Your tileset contains a huge amount of tiles (65536). This is part of the problem because at the moment the terrain tool uses a linear search through the tiles in the tileset to find one that matches a certain terrain criteria. A workaround is to separate your terrain tiles into a smaller tileset (though that can only make it about 100x faster).

The solution would be:

  • Improve the lookup of tiles to not be a linear search through the whole tileset.

  • Improve the algorithm filling the map, such that it searches for the smallest modification and will give up after some time.

Owner

bjorn commented Jul 4, 2017

So, this is due to a combination of factors:

  • Your "oworld1 mountains" terrain only has transitions to "nothing". In your tileset there does not exist any way to connect "oworld1 mountains" to the terrains used by the existing tiles on the "Layer 0 a". This causes it to change the whole map as it futilely tries to fix up the border, which keeps expanding to fill the whole map (which is relatively large). Using "oworld1 mountains" on an empty layer works fine, because the border is immediately fine.

  • Your tileset contains a huge amount of tiles (65536). This is part of the problem because at the moment the terrain tool uses a linear search through the tiles in the tileset to find one that matches a certain terrain criteria. A workaround is to separate your terrain tiles into a smaller tileset (though that can only make it about 100x faster).

The solution would be:

  • Improve the lookup of tiles to not be a linear search through the whole tileset.

  • Improve the algorithm filling the map, such that it searches for the smallest modification and will give up after some time.

@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Jul 4, 2017

Owner

This issue is something to keep an eye on in relation to Wang Tiles (#1455), which @Bdtrotte is working on. Wang Tiles may replace the terrain tool as a more flexible solution and we could make sure that the approach does not suffer from the above issues.

Owner

bjorn commented Jul 4, 2017

This issue is something to keep an eye on in relation to Wang Tiles (#1455), which @Bdtrotte is working on. Wang Tiles may replace the terrain tool as a more flexible solution and we could make sure that the approach does not suffer from the above issues.

@Bdtrotte

This comment has been minimized.

Show comment
Hide comment
@Bdtrotte

Bdtrotte Jul 4, 2017

Contributor

Yeah, I think so too. I haven't thought this far ahead yet... but hopefully finding the transitions will be much faster with the hash maps already in place. I imagine the eventual "wang brush" will be able to match the functionality of the terrain brush

Contributor

Bdtrotte commented Jul 4, 2017

Yeah, I think so too. I haven't thought this far ahead yet... but hopefully finding the transitions will be much faster with the hash maps already in place. I imagine the eventual "wang brush" will be able to match the functionality of the terrain brush

bjorn added a commit that referenced this issue Jul 25, 2017

Limited the area processed by the Terrain Brush
This is mainly useful for making the brush work on infinite maps, but in
general it is good to prevent the tool from going crazy when it doesn't
have a complete set of transition tiles.

The area modified by the tool is now limitd to the maximum distance
between any two terrains, as counted by the number of transitions
needed.

A 'checked' boolean was added to each Cell so that the terrain tool can
use the chunked allocation logic of TileLayer, while still having a
fast place to store whether it processed a certain location or not.

This helps a lot with issue #1632
@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Jul 25, 2017

Owner

While making the Terrain Brush work for infinite maps, I had to apply some kind of limit to the area affected by the brush. This also helped a great deal in resolving this issue, though the wait is still considerable. It is only processing for about 5 seconds on my machine after the above change, which is definitely less fatal.

Owner

bjorn commented Jul 25, 2017

While making the Terrain Brush work for infinite maps, I had to apply some kind of limit to the area affected by the brush. This also helped a great deal in resolving this issue, though the wait is still considerable. It is only processing for about 5 seconds on my machine after the above change, which is definitely less fatal.

bjorn added a commit that referenced this issue Jul 25, 2017

Limited the area processed by the Terrain Brush
This is mainly useful for making the brush work on infinite maps, but in
general it is good to prevent the tool from going crazy when it doesn't
have a complete set of transition tiles.

The area modified by the tool is now limitd to the maximum distance
between any two terrains, as counted by the number of transitions
needed.

A 'checked' boolean was added to each Cell so that the terrain tool can
use the chunked allocation logic of TileLayer, while still having a
fast place to store whether it processed a certain location or not.

This helps a lot with issue #1632

ketanhwr added a commit to ketanhwr/tiled that referenced this issue Jul 28, 2017

Limited the area processed by the Terrain Brush
This is mainly useful for making the brush work on infinite maps, but in
general it is good to prevent the tool from going crazy when it doesn't
have a complete set of transition tiles.

The area modified by the tool is now limitd to the maximum distance
between any two terrains, as counted by the number of transitions
needed.

A 'checked' boolean was added to each Cell so that the terrain tool can
use the chunked allocation logic of TileLayer, while still having a
fast place to store whether it processed a certain location or not.

This helps a lot with issue #1632
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment