Add: Demolish tree tool #7394
Add: Demolish tree tool #7394
Conversation
| } | ||
| } | ||
| } | ||
| break; |
There was a problem hiding this comment.
This whole section is in the wrong place. CMD_CLEAR_AREA already understands orthogonal and diagonal tile areas. Sending a network command for each tile is a bad idea. This should be either a whole new command, or more simply just an additional bit within p2 to specify only trees.
There was a problem hiding this comment.
Oh I didn't know you could manipulate DoCommandP like that, thanks :)
There was a problem hiding this comment.
Wait but I want to keep compatibility with trunk servers - changing/creating commands would adversely affect that right?
There was a problem hiding this comment.
@Hexus-One Writing a patch for trunk and writing a patch compatible with trunk are two entirely different things. As you're making a PR for trunk here you shouldn't be worrying about trunk compatibility as once your request is accepted it will be in the trunk itself. But if for some reason it ends up being rejected you're welcome to make PR for a trunk-compatible client I'm maintaining: https://bitbucket.org/citymania/cmclient
There was a problem hiding this comment.
As @ldpl says, when submitting a PR, if merged into master it will become an official feature, so no need to worry about that.
There was a problem hiding this comment.
you're welcome to make PR for a trunk-compatible client I'm maintaining: https://bitbucket.org/citymania/cmclient
:0 it's the citymania client! I'd love to be included in it :)
There was a problem hiding this comment.
I guess you misunderstood my comment. This should be one DoCommandP(), just like CMD_CLEAR_AREA, just with an extra bit (set in p2) to say "only clear trees".
There should be no loops in the GUI code to send commands.
There was a problem hiding this comment.
Oh, can DoCommandP() be modified like that already? While keeping compatibility with the trunk server?
There was a problem hiding this comment.
Please reread what @ldpl and I wrote?
There was a problem hiding this comment.
I guess I'll try to explain it a bit more. There are two different ways of writing patches:
- Make a patch for OpenTTD master/trunk. It should keep savegame compatibility but not network compatibility. You're never supposed to be able to connect with newer version of the game to older servers. So you should implement your patch in a most logical and clear way. E.g. modifying network commands instead of trying to avoid that. This is a type of patch that you can submit as PR for OpenTTD but you won't be able to use it on older servers.
- Make compatible patch. It can't alter savegames at all and should keep network compatibility. Usually it means doing things in a hacky non-optimal way. Patch like that is not meant to be included into OpenTTD master so it makes no sense to submit it as a PR. But there are some patchpacks like CityMania client that specialize on compatible patches so you can try making PR for them.
Sometimes there are patches that fit both categories but usually that only applies to something cosmetic that doesn't affect actual gameplay.
In general you should aim for your patch to be included in OpenTTD and so follow route 1. Only if it's for some reason rejected you can try second option. Though there is usually a good chance that feature rejected by OpenTTD developers it's not a good feature in a first place so including it in patchpack is also pointless.
In this particular case your patch seems to follow route 2. And it's a fine feature in my opinion, so it can be included in CityMania client assuming code quality is ok (I haven't actually revieved it yet). But as I said you should try to make it included in master first. I generally don't include in CityMania client features that have a good chance to be accepted in master as it's basically a double work.
| @@ -231,4 +231,6 @@ class DiagonalTileIterator : public TileIterator { | |||
| */ | |||
| #define TILE_AREA_LOOP(var, ta) for (OrthogonalTileIterator var(ta); var != INVALID_TILE; ++var) | |||
|
|
|||
| #define DIAGONAL_TILE_AREA_LOOP(var, ta) for (DiagonalTileIterator var(ta); var != INVALID_TILE; ++var) | |||
|
|
|||
There was a problem hiding this comment.
Unnecessary if you fix the command part.
… send less network commands :)
|
I have this bad habit of derailing features with requests for more generic/improved versions of these features. So here we go: I'd like this feature to be more generic/improved, by adding a GUI to the demolish (dynamite) tool, to do tasks like:
|
|
Closing as author has no intention of continuing this, preferring to make a client-side-only patch. |
This demolishes only trees from a given selection. I've yet to change the tooltip and icon properly.