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

Fix #7017: Conditional orders enum updated without savegame version bump #7019

Merged
merged 1 commit into from Jan 6, 2019

Conversation

nielsmh
Copy link
Contributor

@nielsmh nielsmh commented Jan 6, 2019

PR #7017 changed the conditional orders conditions enum by adding a value in the middle. This breaks loading old savegames that use conditional orders. Fix this by moving the new enum value to the end, while keeping the UI list order the same.

Saving this orders list in 1.8.0:
image

Loads as this orders list with the #7017 patch but without this fix:
image

@PeterN
Copy link
Member

PeterN commented Jan 6, 2019

Would be better to avoid a savegame conversion if possible. Can we not just move enum value to the end, and accept that savegames from that specific revision are incorrect?

@nielsmh
Copy link
Contributor Author

nielsmh commented Jan 6, 2019

Main issue with moving the value is UX. The options are presented in definition order in the UI, so moving "max reliability" (the new one) to the end separates it from "(current) reliability".

@PeterN
Copy link
Member

PeterN commented Jan 6, 2019

Well that's just bad UI design :/

@nielsmh
Copy link
Contributor Author

nielsmh commented Jan 6, 2019

No you're right, the order in the UI does not have to match the order in the enum, I just discovered. Going to change the patch to not bump savegame version.

@nielsmh
Copy link
Contributor Author

nielsmh commented Jan 6, 2019

This is definitely a better fix, thanks.

@nielsmh nielsmh merged commit 15a7f9d into OpenTTD:master Jan 6, 2019
@nielsmh nielsmh deleted the fix7017 branch January 6, 2019 10:45
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

2 participants