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

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

src/landscape.cpp Outdated Show resolved Hide resolved
PeterN
PeterN previously requested changes Apr 11, 2019
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 demolish-tool branch 2 times, most recently from 254980a to ae00181 Compare April 11, 2019 21:59
@stormcone
Copy link
Contributor Author

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 demolish-tool branch 3 times, most recently from 929f359 to 78b09fe Compare April 17, 2019 21:34
@stormcone
Copy link
Contributor Author

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.

Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

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

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),
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@Hexus-One
Copy link
Contributor

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

@TrueBrain TrueBrain added candidate: most likely This Pull Request is probably going to be accepted size: large This Pull Request is large in size; review will take a while labels Dec 14, 2020
@stormcone
Copy link
Contributor Author

The toolbar looks like this now:

openttd_demolish_tool

@TrueBrain TrueBrain added the preview This PR is receiving preview builds label Dec 22, 2020
@DorpsGek DorpsGek temporarily deployed to preview-pr-7497 December 22, 2020 18:35 Inactive
@TrueBrain TrueBrain added this to the 1.11.0 milestone Jan 5, 2021
@TrueBrain
Copy link
Member

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: most likely This Pull Request is probably going to be accepted preview This PR is receiving preview builds size: large This Pull Request is large in size; review will take a while
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants