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

Feature: Selective demolition tool. #7497

Closed
wants to merge 1 commit into from

Conversation

@stormcone
Copy link
Contributor

@stormcone stormcone commented Apr 10, 2019

Idea from #7394.

Known bug(?):
The demolish button does not stay lowered because the drop-down list window raises back up when it closes. And I do not know how to keep it down. Maybe I need to get a paperweight. :)

@stormcone stormcone force-pushed the stormcone:demolish-tool branch from 6285a0c to 50440f7 Apr 10, 2019
src/landscape.cpp Outdated Show resolved Hide resolved
src/landscape.cpp Outdated Show resolved Hide resolved
src/terraform_gui.cpp Outdated Show resolved Hide resolved
src/terraform_gui.cpp Outdated Show resolved Hide resolved
src/terraform_gui.cpp Outdated Show resolved Hide resolved
@stormcone stormcone force-pushed the stormcone:demolish-tool branch 2 times, most recently from 254980a to ae00181 Apr 11, 2019
@stormcone
Copy link
Contributor Author

@stormcone stormcone commented Apr 11, 2019

How could be the rising button problem solved? Currently can not abort the demolition if a player clicks on the button again, only with the ESC key.

src/landscape.cpp Outdated Show resolved Hide resolved
src/terraform_gui.cpp Outdated Show resolved Hide resolved
src/terraform_gui.cpp Outdated Show resolved Hide resolved
@stormcone stormcone force-pushed the stormcone:demolish-tool branch 3 times, most recently from 929f359 to 78b09fe Apr 11, 2019
@stormcone
Copy link
Contributor Author

@stormcone stormcone commented Apr 17, 2019

I have reworked the GUI part: instead of a drop-down list, it is now using a small toolbar window. That way the above mentioned problem is solved.

@stormcone stormcone force-pushed the stormcone:demolish-tool branch from 78b09fe to 9fb7be5 Apr 23, 2019
@PeterN PeterN dismissed their stale review May 1, 2019

Outdated

@stormcone stormcone force-pushed the stormcone:demolish-tool branch from 9fb7be5 to 16c3595 May 7, 2019
@stormcone stormcone force-pushed the stormcone:demolish-tool branch from 16c3595 to 0d42e67 Jun 4, 2019
Copy link
Member

@LordAro LordAro left a comment

Code looks more or less fine, not sure about the concept though

src/terraform_gui.cpp Outdated Show resolved Hide resolved
EndContainer(),
NWidget(NWID_HORIZONTAL),
NWidget(WWT_IMGBTN, COLOUR_DARK_GREEN, WID_DM_ALL), SetMinimalSize(22, 22),
SetFill(0, 1), SetDataTip(SPR_IMG_DYNAMITE, STR_DEMOLISH_EVERYTHING_TOOLTIP),

This comment has been minimized.

@LordAro

LordAro Oct 9, 2019
Member

too much indentation here (though I'm not sure what the "expected" amount is, only that 4 tabs is too much)

This comment has been minimized.

@stormcone

stormcone Oct 9, 2019
Author Contributor

I am also not sure how much is expected, but I checked that file when I wrote it, and around line 370 there are the same amount of tabs where the widget attributes are in multiple lines, so I wanted to be consistent with them.

@stormcone stormcone force-pushed the stormcone:demolish-tool branch from 0d42e67 to 82c0007 Oct 9, 2019
@stormcone stormcone force-pushed the stormcone:demolish-tool branch from 82c0007 to 97801b1 Dec 16, 2019
stormcone referenced this pull request in stormcone/OpenTTD Dec 16, 2019
@Hexus-One
Copy link
Contributor

@Hexus-One Hexus-One commented Dec 17, 2019

Hello :) is there any more work that has to be done with this tool?

Idea from #7394.
@stormcone stormcone force-pushed the stormcone:demolish-tool branch from 97801b1 to d7a3ef5 Dec 17, 2020
@stormcone
Copy link
Contributor Author

@stormcone stormcone commented Dec 17, 2020

The toolbar looks like this now:

openttd_demolish_tool

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jan 6, 2021

Hi @stormcone ,

First off, thank you for this PR. Second, sorry it took us this long to get back to you about what we think about this PR :)

Basically, we are all a bit so-so about this functionality. Not because the concept is bad, or the code is bad, but because it only solves a tiny part of a bigger puzzle. Let me explain a bit:

I can fully understand that the current "destroy everything" is not ideal. I often find too that I only want to destroy this or that .. mostly, I mostly do not want to destroy water. So I get where this is coming from.

The implementation however, raises a lot of questions, and we expect our users will have the same. For example, why can I remove trees, but not all stations? Or all town roads? Or only rivers? Or everything except rivers? There are so many combinations and options possible with this, that it is unclear to us what the intentions behind only these 2 options are. For me personally, it wouldn't be enough .. and this is what I am afraid of: that after this PR, we will get a lot of small requests to add this or that to the destroy bar .. ruining the bar in no time :D

So, we will need to look for another way to do this. As the concept is nice, in my opinion, just the way it is implemented will lead to a lot of questions and frustration with our users. How an alternative implementation should look? I really do not know .. if we look at other games, we see that they solve it in all kinds of different ways .. for example, Factorio uses blueprints for that, but we don't have those. So maybe someone can come up with an alternative way to supply this functionality, that does allow all the weird requests people will have "I only want to demolish X" :D

I hope you understand; but I am going to close this PR. As it stands, this PR is just not the solution for us .. I do hope you can come up with an alternative approach, which does allow all this fine-grained control :) If you are interested, I suggest to open up a Discussion to talk over what others think or how we should approach is.

Again, sorry it took us almost 2 years to give you this reply, and I seriously appreciate your work here. But sometimes we have to think about the greater picture here :) Tnx again!

@TrueBrain TrueBrain closed this Jan 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants