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

Allow NewGRFs to give error popups, without disabling them. #9119

Merged
merged 1 commit into from Apr 27, 2021

Conversation

@EratoNysiad
Copy link
Contributor

@EratoNysiad EratoNysiad commented Apr 27, 2021

Fixes #9115 by also creating an error pop-up for errors with the severity ERROR, instead of just FATAL. Also added a string for it, STR_NEWGRF_ERROR_POPUP, which is the string STR_NEWGRF_ERROR_FATAL_POPUP, but without the word "fatal". Also provided a translation in a few languages I'm confident enough in to remove a word.

Motivation / Problem

Say you have a NewGRF, but you should only ever use it with another NewGRF, or a certain combination of settings is completely game breaking. You want to inform the players that they might need to rethink their choices, but you don't want to stop them from going ahead anyway, should they choose to ignore it.
AndyTheNorth raised a similar problem; people kept complaining that they didn't have vehicle newgrfs and they couldn't use FIRS as a result. The ideal solution would have been an error popup to explain to users that they don't have a compatible boat newgrf. The only way of doing this would have been by means of error(FATAL,string(STR_ERROR)); in NML. This would disable the NewGRF, meaning NewGRFs that aren't FIRS compatible would have a hard time becoming FIRS compatible, and that I can't choose to just not have a ship newgrf because I don't use ships.

In NML you can have 4 different types of errors. NOTICE, WARNING, ERROR, and FATAL. You would expect all of these to behave differently in some way, but the only difference between WARNING and ERROR is the red word in the message in the NewGRF window. FATAL gives a popup message, but also disables the NewGRF. Now it would make sense to me if ERROR also gave a pop-up, making it distinct from the other 4.

With this change, NewGRF developers like Andy can tell players "Hey we have detected that you don't have a compatible Road Vehicle NewGRF. This means you will not be able to move goods via truck. Is this correct, and if so are you sure about that?"
For beginning players who are told to play FIRS for whatever reason, this helps by telling them what is wrong. They might not know to look in the NewGRF window, and might miss the message. They might not know that they need a FIRS compatible NewGRF to use it.
For veteran players, this is nice because they might decide that they don't want to use ships anyway, so they can just press [x] and continue playing. Other veteran players might forgot to load a ship newgrf, when they meant to. It's nice to be told this upfront, and to not discover an hour into the game.

The direct motivation for this was a perceived issue with FIRS, but I believe this can be widely applicable.

Description

In newgrf_gui.cpp there's a line of code that prevents a popup from appearing if the severity is not FATAL. I changed it so it also shows a popup if the severity is ERROR.

If the severity is ERROR instead of FATAL it will use a different string, STR_NEWGRF_ERROR_POPUP instead of STR_NEWGRF_ERROR_FATAL_POPUP, which omits the word "fatal" from the error message.

Example:
image

Limitations

The feature itself is fairly complete.

Older NewGRFs might have used ERROR when it didn't give a popup, and loading it with this change will cause them to generate a pop-up. I think that's a very minor problem.

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')
@James103
Copy link
Contributor

@James103 James103 commented Apr 27, 2021

I've provided translations for Afrikaans, Dutch, German, British English and Esperanto.

Translations always must be done using the web translator (https://github.com/OpenTTD/team/issues/new/choose), no exceptions.

@James103
Copy link
Contributor

@James103 James103 commented Apr 27, 2021

When deleting translations that you added as part of a PR, don't delete the whole file, only delete the string that you added. If you need to delete the string in multiple languages, try to do it in a single commit.

@EratoNysiad
Copy link
Contributor Author

@EratoNysiad EratoNysiad commented Apr 27, 2021

I've provided translations for Afrikaans, Dutch, German, British English and Esperanto.

Translations always must be done using the web translator (https://github.com/OpenTTD/team/issues/new/choose), no exceptions.

Alright. How do I delete changes from a PR and how do I add strings to english without changing english.lng? I presume you would need at least 1 string for the thing you want to add.

src/lang/english.txt Outdated Show resolved Hide resolved
@LordAro
Copy link
Member

@LordAro LordAro commented Apr 27, 2021

We have a CONTRIBUTING.md that you may find interesting reading

Copy link
Member

@2TallTyler 2TallTyler left a comment

You will need to rebase your commits into a single commit which follows the commit message format. I suggest "Change: [NewGRF] NewGRFs can create a non-fatal error window"

src/lang/english.txt Outdated Show resolved Hide resolved
src/newgrf_gui.cpp Outdated Show resolved Hide resolved
if (c->error->severity == STR_NEWGRF_ERROR_MSG_FATAL) {
ShowErrorMessage(STR_NEWGRF_ERROR_FATAL_POPUP, INVALID_STRING_ID, WL_CRITICAL);
} else {
ShowErrorMessage(STR_NEWGRF_ERROR_POPUP, INVALID_STRING_ID, WL_CRITICAL);
Copy link
Member

@TrueBrain TrueBrain Apr 27, 2021

Choose a reason for hiding this comment

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

I would guess we shouldn't fire this as WL_CRITICAL. WL_ERROR sounds more appropriate in this case?

@EratoNysiad EratoNysiad force-pushed the ErrorsAreErrors branch 5 times, most recently from 6bedd78 to ff5be1d Apr 27, 2021
…p window now.

Fixed issue OpenTTD#9115 by also creating an error pop-up for errors with the severity ERROR, instead of just FATAL. Also added a string for it, STR_NEWGRF_ERROR_POPUP, which is the string STR_NEWGRF_ERROR_FATAL_POPUP, but without the word "fatal". This makes ERROR functionally distinct from WARNING.
@LordAro LordAro merged commit a934dfe into OpenTTD:master Apr 27, 2021
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants