[patch] tweak to terraform error highlighting

[db48x]

I've written a patch that makes a small change to the way the terraforming code highlights squares that have caused the current terraforming command to fail. Currently the game only highlights the first such square, but I desire it to hilight all these squares.

The patch I'm attaching works perfectly, barring the debug output. However, I did not expect it to work in it's current form; rather I expected that my changes would have broken the error messages. Before I can proceed further, I need someone to take a look at the patch and tell me why it works even when it shouldn't. I may be missing something obvious…

Thanks in advance

[db48x]

Naturally as soon as I tried to go to sleep I figured it out. In case you're wondering, CommandCost::AddCost copies the error state as well as adding the monetary values, so commenting out the early returns didn't actually cause the error states to be lost like I was expecting.

I'll attach another patch in case anyone wants to give it a try, but this is only a part of what I want to do. The goal here is to add a way to tell the terraforming commands to automatically (attempt to) bulldoze whatever is in the way, instead of merely highlighting it. The result is a more streamlined interface. I figure the control key is the way to go.

[Belgabor]

While your aim is clearly a nice feature, I'd suggest rethinking the use of the Ctrl key for it. This is already used in the diagonal terraforming patch, which would then clash with yours.

[db48x]

While your aim is clearly a nice feature, I'd suggest rethinking the use of the Ctrl key for it. This is already used in the diagonal terraforming patch, which would then clash with yours.

Heh, nice. Maybe whoever get it checked in first wins the control key?

[Conditional Zenith]

If this works well, I don't see why the control key is needed at all. If this doesn't work well, it wont get into trunk.

[db48x]

If this works well, I don't see why the control key is needed at all. If this doesn't work well, it wont get into trunk.

Highlighting all of the error squares would happen all the time, control key or not. Auto-bulldozing would only happen when you use the control key, otherwise the results might be very surprising.

As a matter of fact, several people have suggested that I make this two separate patches, one just for highlighting all the error squares and the other for auto-bulldozing. I happen to like that idea, so I'm going with it. I'm attaching an updated patch that adds the two endpoints of a bridge or tunnel that would have to be bulldozed in order for the tf to happen to the list of error squares.

[Conditional Zenith]

Ahh, I missed the second post about auto bulldozing.

To be honest though, if I could either have diagonal level and clear or auto bulldozing on terraform, I would take the former. I don't think many people would argue with highlighting all error tiles though.

[db48x]

Ahh, I missed the second post about auto bulldozing.

To be honest though, if I could either have diagonal level and clear or auto bulldozing on terraform, I would take the former. I don't think many people would argue with highlighting all error tiles though.

Don't worry, they won't conflict. There's no reason why having one should preclude having the other.

[Conditional Zenith]

Except needing more modifier keys than are available. IIRC the current OTTD engine really only supports CTRL. And while I can't speak for everyone, I don't really like the idea of automatically demolishing things.

Also, your patch has at least one hunk where you have only changed whitespace (in src/tilehighlight_type.h, and last hunk in src/viewport.cpp) Please try to avoid that. TerraformClearErrorTiles() needs its whitespace fixed. And I'm assuming PrintErrorTiles() is a temporary debugging function. You should fix line 186 of your diff to be consistent with the code style. Wrong whitespace on line 200.

All these things might seem pedantic, but if you want this to be considered for merging, the devs will ask for these things to be fixed.

[db48x]

Except needing more modifier keys than are available. IIRC the current OTTD engine really only supports CTRL. And while I can't speak for everyone, I don't really like the idea of automatically demolishing things.

Yea, I wouldn't want it to happen automatically on every terraform either. OTTD must also support using shift as a modifier, since that generally allows you to query an action to see how much it'll cost. I'll have to have a look and see what else can be done.

Also, your patch has at least one hunk where you have only changed whitespace (in src/tilehighlight_type.h, and last hunk in src/viewport.cpp) Please try to avoid that. TerraformClearErrorTiles() needs its whitespace fixed. And I'm assuming PrintErrorTiles() is a temporary debugging function. You should fix line 186 of your diff to be consistent with the code style. Wrong whitespace on line 200.

All these things might seem pedantic, but if you want this to be considered for merging, the devs will ask for these things to be fixed.

Indeed. Those are mostly all fixed in the latest version I've attached. The rest I'd already fixed locally. Thanks though, I appreciate your looking at the patch.

[Conditional Zenith]

OTTD must also support using shift as a modifier, since that generally allows you to query an action to see how much it'll cost.

Yes it does, and changing that behaviour will make many people unhappy. IOW, don't touch shift.

[db48x]

OTTD must also support using shift as a modifier, since that generally allows you to query an action to see how much it'll cost.

Yes it does, and changing that behaviour will make many people unhappy. IOW, don't touch shift.

No, you misunderstood. I merely meant that OTTD must support modifiers other than control, because it clearly supports shift as well. I don't intend to replace the existing behavior of the shift modifier. That would be silly.

Added this to Flyspray. http://bugs.openttd.org/task/2719

[Conditional Zenith]

Indeed, it can handle any keypress, it's just that some have a global variable to represent their state (shift and control), and others don't, at the moment they can only really be used as modifiers to keypresses and not modifiers to clicks. See the top of gfx.cpp. And people already complain about the number of features hidden behind control, using another mod key will only increase that.

Looking at the patch you posted on flyspray, you still have some whitespace issues.

Edit: handling other modifiers seems easier than I thought, the remaining question is if it is a good idea to use another one.

[Belgabor]

You also need to keep in mind that openttd is ported to other platforms like game consoles. Having less modifier keys will probably help those people tremendously.
I don't know how feasible this is, but how about being able to activate both a terraforming tool and the demolish tool? Similar to the track/station building tool and the bulldozer.

[db48x]

Indeed, it can handle any keypress, it's just that some have a global variable to represent their state (shift and control), and others don't, at the moment they can only really be used as modifiers to keypresses and not modifiers to clicks. See the top of gfx.cpp. And people already complain about the number of features hidden behind control, using another mod key will only increase that.

Edit: handling other modifiers seems easier than I thought, the remaining question is if it is a good idea to use another one.

You also need to keep in mind that openttd is ported to other platforms like game consoles. Having less modifier keys will probably help those people tremendously.
I don't know how feasible this is, but how about being able to activate both a terraforming tool and the demolish tool? Similar to the track/station building tool and the bulldozer.

Ahh, I see how it works. Belgabor, I really like your idea. I believe you may have solved it.

Looking at the patch you posted on flyspray, you still have some whitespace issues.

Bah, whitespace. Guess I'll go through it all once again.