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: "Remove all industries" button in scenario editor #8550

Merged
merged 1 commit into from Feb 10, 2021

Conversation

@Kuhnovic
Copy link
Contributor

@Kuhnovic Kuhnovic commented Jan 9, 2021

Motivation / Problem

Many scenarios found in on the online content server come with industries. This is unfortunate when you want to play that particular map with for instance the FIRS industry set, or if you want to start with a clean slate for a different reason. The current solution is to go over the entire map and remove each industry, which is tedious.

Description

This feature is only available in the scenario editor. It removes all industries. It also turns removes residual farmland and replaces it with grass.

wAc0puysLU

Limitations

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')
Copy link
Contributor

@glx22 glx22 left a comment

I like this idea. I just quickly checked the code. And made some suggestions.

src/industry_gui.cpp Outdated Show resolved Hide resolved
src/industry_gui.cpp Outdated Show resolved Hide resolved
src/industry_gui.cpp Outdated Show resolved Hide resolved
src/industry_gui.cpp Outdated Show resolved Hide resolved
@TrueBrain TrueBrain changed the title Feature: Remove all industries button in scenario editor Feature: "Remove all industries" button in scenario editor Jan 9, 2021
@Kuhnovic Kuhnovic force-pushed the Kuhnovic:remove_all_industries branch from 5e45870 to 770351b Jan 10, 2021
@Kuhnovic
Copy link
Contributor Author

@Kuhnovic Kuhnovic commented Jan 10, 2021

Thanks for the review Glx22, I addressed your points.

I was also thinking, maybe this is a good opportunity to add a dedicated button for Many Random Industries. I've always found it a bit strange that it was just in the industry list as if it was just another type. Let me know what you guys think.

EDIT: this is what I mean, notice the Many Random Industries entry in the list is also gone.
image

@glx22
Copy link
Contributor

@glx22 glx22 commented Jan 11, 2021

Yeah it may be a good opportunity to remove the many random industries "hack" and use a proper button.
But I think other people might have a different opinion about that.

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jan 11, 2021

I agree, but I properly wouldn't put them on top of each other. Maybe next to each other is better suited? The word "industries" is a bit redundant here even, so, so "Build many random" and "Remove all" might be sufficient. Dunno, just thinking out loud :D

@LC-Zorg
Copy link

@LC-Zorg LC-Zorg commented Jan 11, 2021

Both changes would be very, very useful. They are sorely lacking.

I agree, but I properly wouldn't put them on top of each other. Maybe next to each other is better suited? The word "industries" is a bit redundant here even, so, so "Build many random" and "Remove all" might be sufficient. Dunno, just thinking out loud :D

It doesn't really matter much, but I think the buttons next to each other would look a bit worse due to the asymmetry of the buttons below. Also, full sentences, due to the importance of functions and their effects, seem justified here.

Importantly, what is sorely lacking, and I would very much like to ask, is a confirmation request for both multi-enterprise construction and removal. There is nothing worse than pressing "many random" or "remove all" by accident after a long time creating a scenario ... It will be all the more important that after changing the "Remove all" button will be adjacent to the "Build".

@Kuhnovic Kuhnovic force-pushed the Kuhnovic:remove_all_industries branch from 770351b to 1e3b92f Jan 11, 2021
@Kuhnovic
Copy link
Contributor Author

@Kuhnovic Kuhnovic commented Jan 11, 2021

Importantly, what is sorely lacking, and I would very much like to ask, is a confirmation request for both multi-enterprise construction and removal.

Couldn't agree more. I pushed some additional changes. There's now a Create Random Industries button, I felt that caption was appropriate than "many random industries". Both the Remove and Create buttons ask for user confirmation.

I kept the stacked buttons since I also think it will look strange if the bottom row contains a resize button.

CJ6kITVlqn

@LordAro
Copy link
Member

@LordAro LordAro commented Jan 13, 2021

Looks very good. Removing the "hacky" Create Random Industries "industry" option is always good

I do think the buttons would look better side-by-side though, rather than one above another

@DorpsGek DorpsGek temporarily deployed to preview-pr-8550 Jan 13, 2021 Inactive
@Kuhnovic
Copy link
Contributor Author

@Kuhnovic Kuhnovic commented Jan 13, 2021

I've been trying out some layouts:

image
Stacked, not aligned. Doesn't look right IMO.

image
Resize button on separate line. This can work, but it feels a bit cluttered.

I'm working on a layout that has the resize button next to the build button, but it's difficult to line things up properly and having it look good, while also being able to switch off the scenario-editor-only buttons. This GUI system can be a bit of a pain :P

@LordAro
Copy link
Member

@LordAro LordAro commented Jan 13, 2021

How about with create/remove buttons at the top of the window?

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jan 13, 2021

Honestly, while checking the preview (click the deployment button :D), I think on top of each other like you had it originally is totally fine (I know, I said otherwise earlier .. I changed my mind :P). Your fonts etc are making it look worse for me than it really is:

image

This looks perfectly fine to me :)

(well, maybe all buttons should be on top of each other, but meh, don't think that is for this PR to fix :D)

@Kuhnovic
Copy link
Contributor Author

@Kuhnovic Kuhnovic commented Jan 13, 2021

How about with create/remove buttons at the top of the window?

image
That wouldlook like this. In a way I like the separation, because now the top buttons apply to all industries, where the bottom buttons apply to the selected industry.

(and it's a heck of a lot easier to set up)

This looks perfectly fine to me :)

I'm also fine with keeping the first option of course ;)

@LordAro
Copy link
Member

@LordAro LordAro commented Jan 23, 2021

It's basically a toss-up. Let's go with "top of the window, side by side"

@LordAro LordAro added this to the 1.11.0 milestone Jan 23, 2021
@Kuhnovic Kuhnovic force-pushed the Kuhnovic:remove_all_industries branch from 1e3b92f to c5063fe Jan 23, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-8550 Jan 23, 2021 Inactive
@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jan 31, 2021

image

This is how the window now looks on a clean configuration (so no custom fonts, no font/UI zoom)

@LordAro : sure this is what you want? (honest question, but otherwise, lets merge this!)

Copy link
Member

@TrueBrain TrueBrain left a comment

Some coding-style nitpicks :)

src/industry_gui.cpp Outdated Show resolved Hide resolved
src/industry_gui.cpp Outdated Show resolved Hide resolved
src/industry_gui.cpp Outdated Show resolved Hide resolved
src/industry_gui.cpp Outdated Show resolved Hide resolved
@LC-Zorg
Copy link

@LC-Zorg LC-Zorg commented Jan 31, 2021

One more approach to the layout.

Here, both buttons are below the list of enterprises, in one part with the other function buttons, what at least for me seems to be expected. Unlike previous designs, they are above the part with the explanation of the functions, what seems appropriate.
Fund new industry window v1 0

The arrangement of buttons next to each other reduces the risk of selecting and automatically confirming the wrong function.
On the other hand, the layout with buttons one below the other allows you to minimize the size of the window if necessary. Please note that English names are one of the shorter ones. In many languages, they are much longer, which can increase the size of the window unnecessarily.

Some examples:
Create many random
Monta satunnaista teollisuusaluetta - Finnish
Grunn ghniomhachasan air thuaiream - Scottish Gaelic
Daudz najausi izveidotu razatnu - Latvian

In Polish, the names would also be much longer, but logical abbreviations were used (removing the word "enterprises" which is not needed - just "many random"), so perhaps in other languages ​​these phrases can also be shortened.

@Kuhnovic Kuhnovic force-pushed the Kuhnovic:remove_all_industries branch from c5063fe to 073e894 Jan 31, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-8550 Jan 31, 2021 Inactive
@Kuhnovic
Copy link
Contributor Author

@Kuhnovic Kuhnovic commented Jan 31, 2021

@TrueBrain nitpicking addressed ;)

One more approach to the layout.

Here, both buttons are below the list of enterprises, ....

I agree that it might be better to put the buttons above each other. It already looks a little funky in English tbh, and I expect it to look worse in some "lengthier" languages.

The main reason why I put the buttons above the industry list was to clearly show that they are independent of that list. Putting them below the list could create the impression that the industry selected is somehow going to affect what the buttons do, which is not the case.

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jan 31, 2021

@TrueBrain nitpicking addressed ;)

Did you forget to push them? :D

Putting the buttons as pointed out in the last image makes absolutely no sense to me: requires and produces are part of the industry selected, and have nothing to do with create random and remove all. So yeah .. let's not do that for sure :P

And that settles it: at the top of the window, and on top of each other (not next to each other). It was a toss-up, but latest preview shows one clearly is sub-optimal :D

Tnx for sticking with us @Kuhnovic , and sorry we are all nitpicking like this :D I really appreciate your effort here!

@Kuhnovic Kuhnovic force-pushed the Kuhnovic:remove_all_industries branch from 073e894 to ee45878 Jan 31, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-8550 Jan 31, 2021 Inactive
@Kuhnovic
Copy link
Contributor Author

@Kuhnovic Kuhnovic commented Jan 31, 2021

@TrueBrain nitpicking addressed ;)

Did you forget to push them? :D

Putting the buttons as pointed out in the last image makes absolutely no sense to me: requires and produces are part of the industry selected, and have nothing to do with create random and remove all. So yeah .. let's not do that for sure :P

And that settles it: at the top of the window, and on top of each other (not next to each other). It was a toss-up, but latest preview shows one clearly is sub-optimal :D

Tnx for sticking with us @Kuhnovic , and sorry we are all nitpicking like this :D I really appreciate your effort here!

Forgot to stage my changes. Still getting the hang of git, I'm a mercurial guy ;)

I can tolerate the nitpicking, better to think things through instead of going for a "fix it in the mix" later on :)

I'll swap the orientation of the buttons now and commit that as well. And I'll try not to forget staging my changes this time ;)

@Eddi-z
Copy link
Contributor

@Eddi-z Eddi-z commented Jan 31, 2021

if you find yourself forgetting to stage all the time, you might use "git commit -a" instead, which automatically stages all changes. but there's very religious debates around that.

@Kuhnovic Kuhnovic force-pushed the Kuhnovic:remove_all_industries branch from ee45878 to 0b9379f Jan 31, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-8550 Jan 31, 2021 Inactive
@Kuhnovic
Copy link
Contributor Author

@Kuhnovic Kuhnovic commented Jan 31, 2021

if you find yourself forgetting to stage all the time, you might use "git commit -a" instead, which automatically stages all changes. but there's very religious debates around that.

That sounds a little scary to me, but thanks for the tip!

Everything is pushed now :)

@Kuhnovic Kuhnovic force-pushed the Kuhnovic:remove_all_industries branch from 0b9379f to 5ee17f3 Jan 31, 2021
@TrueBrain TrueBrain merged commit 83ddb15 into OpenTTD:master Feb 10, 2021
8 checks passed
8 checks passed
Emscripten
Details
Commit checker
Details
Check preview needs update Check preview needs update
Details
Linux (clang, clang++)
Details
Linux (gcc, g++)
Details
Mac OS (x64, x86_64) Mac OS (x64, x86_64)
Details
Windows (x86)
Details
Windows (x64)
Details
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