mapeditor/tiled

Automap While Drawing doesn't apply all relevant rules #3313

eishiya posted onGitHub

When Automapping While Drawing, only the rules that look at the layer(s) being edited get applied. If I edit Layer A and a rule updates Layer B in response to a change in Layer A, I expect later rules that only look at Layer B to also apply, but they do not.

It's possible to get around this bug by adding an empty input_Layer A layer in the rule maps, but I don't think this should be necessary.

I've encountered this issue in both the stable builds and in the new automapping branch.


When Automapping While Drawing, only the rules that look at the layer(s) being edited get applied. If I edit Layer A and a rule updates Layer B in response to a change in Layer A, I expect later rules that only look at Layer B to also apply, but they do not.

Hmm, yes I would expect the same. I guess it would be alright though, if this only happened for rules from subsequent rule maps, rather than rules from the same rule map? It would have to work like that in any case in the context of concurrent rule matching.

posted by bjorn about 3 years ago

This issue happens because the filtering of the rule maps happens before any AutoMapping has been done:

https://github.com/mapeditor/tiled/blob/d23131cd156b9006e3d7d2a6eb1020c0768dabb0/src/tiled/automappingmanager.cpp#L122-L138

posted by bjorn about 3 years ago

Hmm, yes I would expect the same. I guess it would be alright though, if this only happened for rules from subsequent rule maps, rather than rules from the same rule map? It would have to work like that in any case in the context of concurrent rule matching.

I think that would be fine, yes. It would be more intuitive if MatchInOrder allowed rules to affect later rules within the same file this way, but it would be fine if it doesn't, as long as you make a note about that in the documentation, perhaps as a warning in the bit on MatchInOrder.

posted by eishiya about 3 years ago

I looked into fixing this for Tiled 1.8.3, but the changes had too much overlap with the automapping branch, so it will be fixed in Tiled 1.9.

I guess it would be alright though, if this only happened for rules from subsequent rule maps, rather than rules from the same rule map?

While implementing this, I realized that this distinction is not relevant here, because the skipping happens on a per-rule-map basis and not per-rule. If a rule map does not have a given modified layer as input, it can never happen, that some of its rules would end up being relevant indirectly.

There remains one case where this optimization breaks things though, which is for example when some rules depend on some other rules placing certain tiles on some condition that isn't related to the edited layers, but which do need to be placed every time again for future rules to process correctly. Just let me know if you run into such a case, then I guess we'll just add a rule map property like "NeverSkip" or "AlwaysApply".

I think the optimization is still good by default, since in general it can greatly speed up or entirely prevent needless AutoMapping runs.

posted by bjorn about 3 years ago

There remains one case where this optimization breaks things though, which is for example when some rules depend on some other rules placing certain tiles on some condition that isn't related to the edited layers, but which do need to be placed every time again for future rules to process correctly. Just let me know if you run into such a case, then I guess we'll just add a rule map property like "NeverSkip" or "AlwaysApply".

I can imagine this being an issue for rules that require some sort of guide layer, but where the guide layer itself may need automapping before it can be properly used. This really should be very rare though, and I feel like in most such cases, one would want to do a full round of Automapping on such layers before using Automap While Drawing anyway. If a map property is added, I think NeverSkip would be a more informative name than AlwaysApply.

posted by eishiya about 3 years ago

Fund this Issue

$0.00
Funded

Pull requests