updated automapper.cpp #1524

Open
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@axboureau

I also added some comments (maybe some of them are too"trivial"?).

updated automapper.cpp
- A match won't fail if an input_layer has no corresponding layer in the workmap as long as all of the cells of the input_layer for the region are empty. I added a dummy empty layer that gets passed to compareLayerTo 
- A match won't fail if one cell of an input_region is empty and the corresponding workmap layer cell is also empty (it will also work if the cell of the workmap layer is not empty). cf change line 752.
It should address the problems mentioned here:
 http://discourse.mapeditor.org/t/trying-to-understand-solve-an-issue-with-automapping-hope-someone-can-help/2228/2
http://discourse.mapeditor.org/t/automap-rules-input-multiple-layers/438/15
and here:
#1520

I also added some documentation (maybe it is too"trivial"?)
@bjorn

You're indeed rather verbose with the comments you've added, but I don't mind so much. I've mentioned two issues inline though.

+ dummyCreated = true;
+ setLayer = new TileLayer(name, 0, 0,
+ mMapWork->width(),
+ mMapWork->height());

This comment has been minimized.

@bjorn

bjorn May 8, 2017

Owner

This is going to be a major performance impact. AutoMapper::applyRule is called for each rule for each location of the map, so creating and throwing away a tile layer is rather too expensive to do here. At the least the dummy should be created once in advance and re-used, but maybe we could just pass in a null pointer and handle this case in compareLayerTo?

Or, judging by how you described this logic in the commit message, you could add an explicit isRegionEmpty check and perform it on all conditions.listYes.

@bjorn

bjorn May 8, 2017

Owner

This is going to be a major performance impact. AutoMapper::applyRule is called for each rule for each location of the map, so creating and throwing away a tile layer is rather too expensive to do here. At the least the dummy should be created once in advance and re-used, but maybe we could just pass in a null pointer and handle this case in compareLayerTo?

Or, judging by how you described this logic in the commit message, you could add an explicit isRegionEmpty check and perform it on all conditions.listYes.

This comment has been minimized.

@axboureau

axboureau May 8, 2017

Indeed, that is a good point.

@axboureau

axboureau May 8, 2017

Indeed, that is a good point.

@@ -749,7 +797,7 @@ static bool compareLayerTo(const TileLayer *setLayer,
if (listNo.isEmpty()) {
if (matchListYes)
continue;
- if (!ruleDefinedListYes && !cells.contains(c1))

This comment has been minimized.

@bjorn

bjorn May 8, 2017

Owner

This change would require an update to the comments, in particular this part:

 *      Exception, when having only the listYes:
 *      if at the examined position there are no tiles within all Layers
 *      of the listYes, all tiles except all used tiles within
 *      the layers of that list are considered good.
 *
 *      This exception was added to have a better functionality
 *      (need of less layers.)

And apparently, this makes some case worse. Essentially, leaving a tile empty in the listYes is taken to mean that there should not be any tile used elsewhere in that same rule (when there are no explicit exclusion rules). Without that logic you need to fill the listNo with all those tiles to get the same functionality. That seems like a nice shortcut, but it is indeed rather unexpected. Maybe @stefanbeller could weigh in on this?

@bjorn

bjorn May 8, 2017

Owner

This change would require an update to the comments, in particular this part:

 *      Exception, when having only the listYes:
 *      if at the examined position there are no tiles within all Layers
 *      of the listYes, all tiles except all used tiles within
 *      the layers of that list are considered good.
 *
 *      This exception was added to have a better functionality
 *      (need of less layers.)

And apparently, this makes some case worse. Essentially, leaving a tile empty in the listYes is taken to mean that there should not be any tile used elsewhere in that same rule (when there are no explicit exclusion rules). Without that logic you need to fill the listNo with all those tiles to get the same functionality. That seems like a nice shortcut, but it is indeed rather unexpected. Maybe @stefanbeller could weigh in on this?

This comment has been minimized.

@axboureau

axboureau May 8, 2017

Maybe we should add some other layer types (on top of regions, inputyes, inputnot) to handle such cases. Leaving all listYes empty is sometimes necessary when we want to create a region rule where not all input tiles are adjacent(like an X shaped one ), so I think it is better if an empty input tile has the expected result, and use some additional explicit layers for other cases.

@axboureau

axboureau May 8, 2017

Maybe we should add some other layer types (on top of regions, inputyes, inputnot) to handle such cases. Leaving all listYes empty is sometimes necessary when we want to create a region rule where not all input tiles are adjacent(like an X shaped one ), so I think it is better if an empty input tile has the expected result, and use some additional explicit layers for other cases.

This comment has been minimized.

@bjorn

bjorn May 8, 2017

Owner

@axboureau I agree the system should work more intuitively, but do you have a specific suggestion of what alternative to offer for people relying on this behavior?

@bjorn

bjorn May 8, 2017

Owner

@axboureau I agree the system should work more intuitively, but do you have a specific suggestion of what alternative to offer for people relying on this behavior?

This comment has been minimized.

@axboureau

axboureau May 8, 2017

Indeed, maybe the best would be to have a property to handle the way it works when there is no inputnot_layer.
Otherwise, we'd have to add an empty inputnot_layer for each input_layer.

@axboureau

axboureau May 8, 2017

Indeed, maybe the best would be to have a property to handle the way it works when there is no inputnot_layer.
Otherwise, we'd have to add an empty inputnot_layer for each input_layer.

@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Dec 31, 2017

Owner

A match won't fail if an input_layer has no corresponding layer in the workmap as long as all of the cells of the input_layer for the region are empty. I added a dummy empty layer that gets passed to compareLayerTo

@axboureau I've addressed this in change a22476f, which does essentially the same without allocating and freeing a lot of memory all the time (it certainly helps that tile layers are infinite now and only allocate chunks of memory on-demand).

A match won't fail if one cell of an input_region is empty and the corresponding workmap layer cell is also empty (it will also work if the cell of the workmap layer is not empty). cf change line 752.

I've tried to make this change, but it changed the behavior in a way that broke the "sewer_automap" example. It might still be that this behavior is desired in more cases, but I didn't want to break compatibility at this point. I'm hopeful that the new "StrictEmpty" mode would also solve your use-case, see the description of change eb1e850 for more information.

If that new option resolves your use-case, then I think we can close this PR and issue #1520. But I would also still be open to introduce another option for this if the option doesn't turn out to be so convenient (which I could definitely imagine).

Owner

bjorn commented Dec 31, 2017

A match won't fail if an input_layer has no corresponding layer in the workmap as long as all of the cells of the input_layer for the region are empty. I added a dummy empty layer that gets passed to compareLayerTo

@axboureau I've addressed this in change a22476f, which does essentially the same without allocating and freeing a lot of memory all the time (it certainly helps that tile layers are infinite now and only allocate chunks of memory on-demand).

A match won't fail if one cell of an input_region is empty and the corresponding workmap layer cell is also empty (it will also work if the cell of the workmap layer is not empty). cf change line 752.

I've tried to make this change, but it changed the behavior in a way that broke the "sewer_automap" example. It might still be that this behavior is desired in more cases, but I didn't want to break compatibility at this point. I'm hopeful that the new "StrictEmpty" mode would also solve your use-case, see the description of change eb1e850 for more information.

If that new option resolves your use-case, then I think we can close this PR and issue #1520. But I would also still be open to introduce another option for this if the option doesn't turn out to be so convenient (which I could definitely imagine).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment