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

Add setTerrain functionality to TileLayerEdit #3758

Merged
merged 18 commits into from
Jun 28, 2023

Conversation

a-morphous
Copy link
Contributor

@a-morphous a-morphous commented May 31, 2023

This adds a new function setTerrain to TileLayerEdit, which can be called by extensions. The setTerrain function operates very similarly to how the stamp brush works when working with wang terrain sets, and is meant for use cases like procedurally generating maps using terrain sets.

This is implemented by creating a new wangpainter class that wangbrush now uses functionality from that handles the creation of wang tiles, which can also be used outside of the brush engine to programmatically set terrain. There's also now a new TileLayerWangEdit class, which can be used to create terrain edits to a tile layer, similar to how TileLayerEdit works for regular tile edits.

This allows extensions to use the following API

const editor = currentLayer.wangEdit(wangSet)
// setTerrain(x, y, color, direction (optional for corner wang sets))
editor.setTerrain(1, 0, 1)
editor.setTerrain(1, 1, 1)
editor.setTerrain(1, 2, 1)
editor.setTerrain(2, 3, 1)
editor.apply()
image

setTerrain's usage is fairly straightforward for corner wang tiles. For edge and edge/corner, the direction needs to be set because of how the stamp works, and it's not very intuitive.

For instance, to get this shape:
image
You'd need to call the following code:

// typings for direction
enum Direction {
	Top = 0,
	TopRight = 1,
	Right = 2,
	BottomRight = 3,
	Bottom = 4,
	BottomLeft = 5,
	Left = 6,
	TopLeft = 7,
}

// TileLayerWangEdit.setTerrain(x, y, colorIndex, direction)
editor.setTerrain(3, 3, 1, Direction.Right); 
editor.setTerrain(4, 3, 1, Direction.Left);
editor.setTerrain(3, 5, 1, Direction.Top); 
editor.setTerrain(3, 4, 1, Direction.Top);

Any feedback at all is welcome, from the shape of the API to implementation details.

(Modified to reflect latest state of the pull request.)

@eishiya
Copy link
Contributor

eishiya commented May 31, 2023

I don't know enough about the code to offer useful feedback, but:

  1. Thank you for looking into this! This would make some of my scripts much simpler and better.
  2. I don't think you need to implement symmetry, since that's something that would be simple to implement via the script if needed. I think it's just a matter of calling setTerrain again on (map.width - 1 - x, map.height - 1 - y).
  3. Why is direction not required for setting the corner? It doesn't usually make sense to set Terrain for an entire Tile, one usually sets it on one corner. A built-in option to set all relevant directions at once would be nice though, similar to the Ctrl option on the Terrain Brush. Perhaps this could be a special value for direction, the exact meaning of which changes based on the effective type of that terrain (e.g. set all corners for a Corner terrain, all Edges for a Corner terrain, and for Mixed terrains, it should use the effective type of the terrain to decide whether to set Corners, Edges, or both).
  4. I feel the parameter order is weird and unintuitive, I think x, y, direction, WangSet, color would be more intuitive - "color" means nothing until the WangSet is specified, so it makes sense for the WangSet to come first. Similarly, the direction is really just part of the location where the terrain is set, and makes sense being grouped with x and y. It would probably also be a good idea to provide an enum in WangSet for the directions, so a user could write something like setTerrain(x, y, WangSet.TopLeft, terrain, 1). In my own existing code dealing with WangSets, I've been implementing JS-side enums like this and it's been great for code legibility. I imagine direction is currently last because it's optional, but as mentioned above, I don't think it makes sense to allow the direction to be unset.
  5. You mentioned that the code doesn't currently remember the desired terrains between calls. While this isn't strictly necessary, having them be remembered would be very interesting, as it would make it much easier to create expansive areas of terrains using incomplete terrain sets. For example, if I want to draw a 3x3 with setTerrain with a corner terrain that's missing concave corners, I'd try something like this (using named directions for readability):
editor.setTerrain(3, 3, 1, terrain, WangSet.BottomRight);
// would modify tiles 3,3, 4,3, 3,4, and 4,4 to create a 2x2 terrain out of four convex corners

editor.setTerrain(5, 3, 1, terrain, WangSet.BottomLeft);
// would expand the shape above into a 3x2 rectangle with two sides and the four convex corners

editor.setTerrain(3, 5, 1, terrain, WangSet.TopRight);
// *should* expand the shape above into a theoretical sideways L shape,
// but this would require a concave corner tile, which we don't have.
// At present, this will probably produce garbage (or, if we're lucky, the final result we want).
// But if the function doesn't compute the tiles for the desired terrains until
// TileLayerEdit.apply() is called, then there would be no issue.

editor.setTerrain(5, 5, 1, terrain, WangSet.TopLeft);
// Finish up the square by placing the final corner. Ideally, this should just give us our square.
// But if the previous step tried to place actual tiles before we finished defining our desired shape,
// then this bit might do nothing, or it might be polishing a turd :]

While a paint-whole-tile mode like Ctrl on the Terrain Brush could help with the above 3x3 rectangle scenario, it would not be enough for rectangles that are larger than 3x3 in both directions, as that would still require an intermediate shape with a concave corner.
If this WangPainter could accept a whole bunch of terrain changes and and only calculate the actual final terrains at the end upon "apply", then it would also make it much easier to implement things like single-terrain bucket fill (#3614) and terrain shape tools (#3047), and it could open up the door to many more interesting features, such as terrain lines, terrain shape stamps, and so on.

@a-morphous
Copy link
Contributor Author

a-morphous commented May 31, 2023

@eishiya, to answer your questions:

Directions for corner

The reason why directions aren't required for corner is because that's true for the behavior of the wang brush when using it in the map editor; the direction is always set to TopLeft. This is because when painting using a corner tileset, the coordinate value for painting is defined to be at that corner in the tilemap, and then the brush paints the four corner tiles around that coordinate.

As an example, a single setTerrain call at position (1, 1) for a corner produces the following:
image

This behavior does sometimes differ from the terrain stamp, because the logic there also takes into account the mouse position between tiles to possibly shift the position that new tiles should be drawn in, and thus how the terrain is painted. As the setTerrain call won't have a mouse to make these small positional changes, I'm not entirely sure how else to handle it -- the direction is always set to TopLeft either way.


Meanwhile, for edge or edge-and-corner based terrains, the brush will change directions when you paint with the mouse depending on where the mouse is relative to the center of a tile in the tilemap. So, a call to setTerrain at position (1,1) will produce the following with a direction of WangIndex::Bottom:
image

and this with a direction of WangIndex::Right:
image

...at least theoretically. This works on my local build, but when trying it with the build produced by Github Actions, the direction isn't being set properly from the extension, and is always Left. I'm not sure why this is the case, and I'll need to investigate.

Parameter order

The idea that wangSet should come before color makes sense, and I can amend that. I'm less sure about direction, since as mentioned above it really is optional for corner, and definitely not optional for edge based tilesets. The simplest API might be to make wangSet and color the first two parameters, and then x y and direction -- but that then no longer matches with how setTile works.

I'm still thinking on this.

Remembering terrain between calls

The example you gave with incomplete concave tilesets will indeed break with my code as written...sometimes; sometimes the end result is the 3x3 square you want, and sometimes the end result is
image

Trying to make it so all terrain is set at the very end is going to require a total rewrite, though, since right now I'm placing terrain using the exact same logic as the existing terrain brush, which calculates all the edges as it's generated. I'm actually kind of confused how these incomplete tileset cases are handled in the map editor; when painting terrain from within Tiled using an incomplete tileset, trying to draw the square, sometimes I get the square as expected, and sometimes I get shapes that overwrite previous tiles and are not squares:

image

I suspect that once again, the lack of a relative mouse position to the current tile is causing a difference in behavior from what the API is capable of vs what you get when using the wang brush tool in editor.

ADDENDUM: The tests I did above were with a weird incomplete terrain set where I'd set the following corners:
image
...which behave strangely both in editor and with my code.

If you have an incomplete tileset that just has the exterior corners, e.g.
image
the 3x3 square actually does get generated properly in your example. More complex generations still break, though, and I don't know if fixing that is feasible with the current generation techniques; the existing wang brush tool also behaves very oddly when you work with incomplete sets.

@eishiya
Copy link
Contributor

eishiya commented May 31, 2023

The current Terrain code in Tiled, when it can't find a perfect match for desired terrains at a location, will modify some of the terrains/tiles to try to get something that makes sense overall, even if it's not quite what the user asked for. It does a search with some modified terrains I think, and the attempted modifications are random, which is why you get different results. On top of this, in the interest of performance, the search is not exhaustive, so it can end up with completely invalid scenarios too.

Regarding remembering terrain between calls / dealing with incomplete terrains: Indeed, this would require a lot of rewriting, and I think it should be tackled along with the two issues I linked. The WangPainter you added is a great idea IMO, and I think that rather than duplicating the existing code, it might be best to tackle the existing Terrain issues, and the resulting improved WangSet functionality could then be exposed to scripting using whatever parameters are deemed appropriate once the new code is in place. In addition to the two issues I linked above, another issue that would benefit from the ability to compute terrains for more than a tile at a time is #2035.
I don't expect you to tackle three issues that aren't even directly related to what you're trying to achieve, rather I think that perhaps this particular change (adding setTerrain to scripting) should be added later, when the terrain internals are improved.

@bjorn
Copy link
Member

bjorn commented May 31, 2023

First of all I agree it's a nice idea to expose the terrain painting functionality to the scripting API!

Since it's already getting late here I'll only note two things.

  • It would indeed be great to share the base code between the scripting API calls and the WangBrush. I understand a lot of state was introduced to not modify the code copied from WangBrush::updateBrushAt too much, but I think the preferred solution will be to reduce some of that state, and possibly to move some of it, like the FillRegion and the function(s) to modify it, into the WangFiller.

  • I think it would be best to keep TileLayerEdit as-is and to introduce a separate TileLayerWangEdit for example. Especially since currently, terrain operations always work using a single WangSet, we can do let wangEdit = tileLayer.wangEdit(wangSet) and avoid repeating that parameter. Then, the while business with picking the right tiles can wait until wangEdit.apply() is called, which would internally call WangFiller::fillRegion and apply the result to the target layer (we'll probably still want to find a way to share the code from TileLayerEdit::apply for this). It's a suggestion, so feel free to let me know if you think it's a bad idea for some reason. :-)

(and boy am I starting to hate the naming inconsistency between "wang" and "terrain" now...)

@a-morphous
Copy link
Contributor Author

Some updates, still not ready to be merged yet. I've addressed some, but not all of the concerns that were laid out above:

  • FillRegion moved to wangfill.
  • wangpainter API changed so that it, as suggested, creates a FillRegion of all desired changes, whereupon you can run the commit function to turn it all into terrain tiles and paint them onto a tile layer. @eishiya, is this consistent with what you were asking for?
  • The updateBrushAt function in wangbrush was moved to wangpainter and renamed, because that function really is the core of how wang terrain is generated, and is necessary for all uses of wangpainter. This enabled a small refactor of wangbrush where it calls into a wangpainter instance to populate its FillRegion for creating the stamp.
  • (INCOMPLETE) the changes in tilelayeredit now reflect these changes -- when calling setTerrain, it only modifies the wang ids for the fill region, and all the changes are baked onto the tile layer only when apply() is called.

Still unaddressed

  • I still don't really know what mIsTileMode is supposed to mean in wangbrush, and right now it's set to false. There should be a way to access it considering that wangbrush actually uses it.
  • @bjorn's suggestion to introduce a separate TileLayerWangEdit. I'm not opposed to adding it, it just hasn't happened yet.
  • API edits. New API I've added may also have their names changed because quite frankly I didn't really think too hard about any of the names I've chosen. Some points of contention are how WangId::Index variables are named; I consistently name them 'direction' because that made sense to me, but I don't think this is used anywhere else in the code.

It shouldn't take too too much time to get the rest of the concerns addressed, but this is all for today.

@a-morphous a-morphous force-pushed the expose-set-terrain branch from 4ae0c97 to 2fd002e Compare June 2, 2023 23:40
@a-morphous a-morphous force-pushed the expose-set-terrain branch from 2fd002e to 532edce Compare June 2, 2023 23:41
@a-morphous
Copy link
Contributor Author

a-morphous commented Jun 2, 2023

I think at this point, I've implemented all the requested changes. The latest changes implements bjorn's suggestion to create a new TileLayerWangEdit class, and also refactors any code duplication.

So now to edit terrain in an extension you'd do something like this (in Typescript):

// typings for direction
enum Direction {
	Top = 0,
	TopRight = 1,
	Right = 2,
	BottomRight = 3,
	Bottom = 4,
	BottomLeft = 5,
	Left = 6,
	TopLeft = 7,
}

const editor = currentLayer.wangEdit(wangSet)
// setTerrain(x, y, color, direction)
editor.setTerrain(1, 1, 1, Direction.Right)
editor.setTerrain(3, 3, 1, Direction.Top)
editor.setTerrain(6, 3, 1, Direction.Left)
editor.setTerrain(10, 3, 1, Direction.Top)
editor.apply()

I'm also marking this PR 'ready for review' since all the functionality is here and I've tested it to the best of my ability, and will need feedback from maintainers to make sure it's suitable to be merged. Specifically:

  • Any suggestions to API changes
  • Whether or not I refactored / moved code to the correct places
  • How to add documentation / tests / types

@a-morphous a-morphous marked this pull request as ready for review June 2, 2023 23:50
src/tiled/wangpainter.cpp Outdated Show resolved Hide resolved
bjorn added 9 commits June 14, 2023 22:37
* Moved part of WangPainter back to WangBrush (BrushMode / TileMode).
  Tile mode could be re-introduced in WangPainter, if it is useful
  enough for a scripting API function.

* Merged the remainder of WangPainter with WangFiller, renaming the
  result to WangPainter. It did not seem like there was any advantage to
  separating these.

* Fixed leaking WangPainter instance in TileLayerWangEdit.
Even though I like WangPainter more, I'm sticking with WangFiller for
now to keep the patch easier to read.
* Exposed WangId as Q_GADGET, so that in JS we can do WangId.Top, etc.
  to specify Wang indexes. Also made the masks enum available, though
  it may not have a use with the current API. A WangId value is
  currently useless on the script side.

* Changed the parameter order to be consistently: position, index, color.

* Fixed registration of TileLayerWangEdit (needed for Qt 5).

* Added nullptr check to TileLayer.wangEdit.

* Added error when calling TileLayerWangEdit.setEdge with non-edge index.

* Added overloads to TileLayerWangEdit functions taking QPoint, which
  might be convenient in some cases.

* Renamed TileLayerWangEdit.setTerrain to setWangIndex, to better reflect
  its low-level behavior.
Instead of WangId.Top, you now type WangIndex.Top in JS. This allowed
for more intuitively using the WangIndex type as a function parameter.

Also exposed the "corrections enabled" setting of WangFiller, which
could be useful.

Finally, updated the JS API docs.
* Destroy the TileLayerWangEdit when the EditableWangSet is deleted.
  This might be too early in some cases, since the WangSet might still
  be alive, but it's the safest option for now.

* Support creating a TileLayerWangEdit even when there is no
  MapDocument. The layer still has to be part of a map though, since we
  need to know the orientation. A check has been added to wangEdit().

* Protect against issues when the originally referred map is deleted by
  copying its parameters (though not sure about the exact flow, which
  might already delete the EditableTileLayer).
@bjorn
Copy link
Member

bjorn commented Jun 26, 2023

@a-morphous I think this change is ready to go in now, but I will await your feedback. It could really use some more testing through the scripting APIs. I've mainly just tested that the existing Terrain Brush and Terrain Fill modes still work, that it compiles and runs against Qt 5 and such, and testing a very simple script:

ws = tiled.mapEditor.currentWangSet
we = tiled.activeAsset.currentLayer.wangEdit(ws)
we.setCorner(5, 5, tiled.mapEditor.currentWangColorIndex)
we.apply()

@a-morphous
Copy link
Contributor Author

I have not yet dug too deeply into the code to figure out why, but the scripting API isn't always producing correct terrain results on a local build of Tiled.

This code:

let ws = tiled.mapEditor.currentWangSet
let editor = tiled.activeAsset.currentLayer.wangEdit(ws)
editor.setCorner(1, 1, tiled.mapEditor.currentWangColorIndex)
editor.setCorner(3, 3, tiled.mapEditor.currentWangColorIndex)
editor.setCorner(6, 3, tiled.mapEditor.currentWangColorIndex)
editor.setCorner(10, 3, tiled.mapEditor.currentWangColorIndex)

editor.apply()

produced this result, with a corner-only tileset:
image

This does seem to be a rare case; This problem does not occur if correctionsEnabled is set to true, nor does it occur on the Github Actions MacOS build; I only see it on an m1 local build. More complex cases, like the procedural generation I was doing, works correctly.

When corrections are disabled, and the Wang set is not complete, a
matching tile is only placed when it will not make it impossible to
place a matching tile on any of its neighboring cells.

For incomplete Wang sets with transitions to empty, it can happen that a
matching tile requires an empty WangId as neighbor. However, the empty
WangId is never explicitly part of a WangSet. As such, we should
probably never discard a match when it requires an empty neighbor.
@bjorn
Copy link
Member

bjorn commented Jun 27, 2023

I only see it on an m1 local build.

Hmm, actually I saw the same problem in my local build on Linux. I've investigated the issue and I think I found the right solution. It seems to have been an existing bug, which only triggers for Wang sets that contain transitions to empty when used without having corrections enabled (and then only in cases like above, where two modified corners are close but not touching...). Good job catching this one! :-)

Since the TileLayerWangEdit allows the WangFiller to be reused, we need
to make sure it starts off with a clean state.
@bjorn bjorn merged commit fcd9733 into mapeditor:master Jun 28, 2023
@bjorn
Copy link
Member

bjorn commented Jun 28, 2023

@a-morphous Thank you so much for getting this change started! I think it is a very useful addition which will hopefully also allow @eishiya to eliminate much of the code from https://github.com/eishiya/tiled-scripts/blob/main/Terrain%20Rectangle%20Tool/TerrainRectangleTool.js.

I intend to make a Tiled 1.10.2 release soon, but I'll wait a bit on further feedback to allow for small fixes / additions.

@eishiya
Copy link
Contributor

eishiya commented Jun 28, 2023

Is the new API documentation available anywhere easy-to-read, or is the diff of index.ts.d the best option right now? I'd love to try it out for that script and maybe some WIPs I have.

Edit: Since TileLayerWangEdit is for a specific layer, making a Tool that previews the changes is rather annoying. I can't think of any way to use this for a Tool with a preview that doesn't also duplicate the entire working layer (since the affected area isn't known ahead of time in the case of incomplete terrain sets + corrections enabled) and then remove the unchanged tiles ):
While my current approach is rather lengthy since it reimplements some Terrain logic, it lets me sample current terrains from one the working layer and place the result in the preview layer, so it makes it easy and performant to preview the result.

Perhaps there should be an alternative to apply() that calculates the terrain tiles, but returns the affected area as a new TileLayer instead of applying it directly to the layer? Maybe it can be called generate() or something. Then, the script can put that layer in the preview for previewing, and later map.merge(preview). apply() would still be useful for scripts that don't need to preview their work, such as Actions.

@bjorn
Copy link
Member

bjorn commented Jun 28, 2023

Perhaps there should be an alternative to apply() that calculates the terrain tiles, but returns the affected area as a new TileLayer instead of applying it directly to the layer? Maybe it can be called generate() or something.

Hmm, that makes a lot of sense and should be easy to add. Alternatively, maybe we could have an applyTo(tileLayer) version.

@eishiya
Copy link
Contributor

eishiya commented Jun 28, 2023

Something like applyTo(tileLayer) would be convenient, but then it might make it sound like it'll take that layer's existing tiles into account, instead of the source layer's. In addition, there might be a lack of clarity about whether this will clear the layer before adding the modified tiles, whether it'll resize the layer, etc.

@bjorn
Copy link
Member

bjorn commented Jun 28, 2023

Is the new API documentation available anywhere easy-to-read, or is the diff of index.ts.d the best option right now? I'd love to try it out for that script and maybe some WIPs I have.

I've re-generated the scripting docs, so they are now available at https://www.mapeditor.org/docs/scripting/interfaces/TileLayerWangEdit.html

I'll look into adding the generate function tomorrow.

@bjorn
Copy link
Member

bjorn commented Jun 29, 2023

I'll look into adding the generate function tomorrow.

An initial version of this is now proposed at #3770.

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.

3 participants