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: Demolish tree tool #7394

Closed
wants to merge 2 commits into from
Closed

Add: Demolish tree tool #7394

wants to merge 2 commits into from

Conversation

Hexus-One
Copy link
Contributor

This demolishes only trees from a given selection. I've yet to change the tooltip and icon properly.

@Hexus-One Hexus-One changed the title Added demolish tree tool Add: Demolish tree tool Mar 21, 2019
}
}
}
break;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I didn't know you could manipulate DoCommandP like that, thanks :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait but I want to keep compatibility with trunk servers - changing/creating commands would adversely affect that right?

Copy link
Contributor

@ldpl ldpl Mar 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @ldpl says, when submitting a PR, if merged into master it will become an official feature, so no need to worry about that.

Copy link
Contributor Author

@Hexus-One Hexus-One Mar 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ldpl

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 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, can DoCommandP() be modified like that already? While keeping compatibility with the trunk server?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please reread what @ldpl and I wrote?

Copy link
Contributor

@ldpl ldpl Mar 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I'll try to explain it a bit more. There are two different ways of writing patches:

  1. 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.
  2. 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.

src/terraform_gui.cpp Outdated Show resolved Hide resolved
@@ -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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary if you fix the command part.

@Eddi-z
Copy link
Contributor

Eddi-z commented Mar 21, 2019

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:

  • only clear tram rails (leave roads and stations alone)
  • only clear your own property (leave houses and town roads alone)

@PeterN
Copy link
Member

PeterN commented Apr 7, 2019

Closing as author has no intention of continuing this, preferring to make a client-side-only patch.

@PeterN PeterN closed this Apr 7, 2019
stormcone added a commit to stormcone/OpenTTD that referenced this pull request Apr 10, 2019
stormcone added a commit to stormcone/OpenTTD that referenced this pull request Apr 10, 2019
stormcone added a commit to stormcone/OpenTTD that referenced this pull request Apr 11, 2019
stormcone added a commit to stormcone/OpenTTD that referenced this pull request Apr 11, 2019
stormcone added a commit to stormcone/OpenTTD that referenced this pull request Apr 11, 2019
stormcone added a commit to stormcone/OpenTTD that referenced this pull request Apr 12, 2019
stormcone added a commit to stormcone/OpenTTD that referenced this pull request Apr 17, 2019
stormcone added a commit to stormcone/OpenTTD that referenced this pull request Apr 23, 2019
@Hexus-One Hexus-One deleted the demotree branch May 4, 2019 18:35
stormcone added a commit to stormcone/OpenTTD that referenced this pull request May 7, 2019
stormcone added a commit to stormcone/OpenTTD that referenced this pull request Jun 4, 2019
stormcone added a commit to stormcone/OpenTTD that referenced this pull request Oct 9, 2019
stormcone added a commit to stormcone/OpenTTD that referenced this pull request Dec 16, 2019
stormcone added a commit to stormcone/OpenTTD that referenced this pull request Dec 17, 2020
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.

None yet

4 participants