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

Add more tree placement options to non-scenario saves. #743

Open
wants to merge 10 commits into
base: jgrpp
Choose a base branch
from

Conversation

tabytac
Copy link

@tabytac tabytac commented Sep 6, 2024

Motivation / Problem

The tree tools in the scenario editor are much more useful than those available in the base game. This PR simply shows the buttons to enable users to use these tools also in standard games.

Description

This is the least amount of code touched way of adding the "Normal", "Grove" and "Forest" buttons to .sav games from .scn games.
This doesn't (at least it shouldn't) affect anything to do with the tree tool in the scenario editor

image

Limitations

  • Grove and Forest options do not have a cost when using the tool.
  • Other attributes or stats also might not be configured correctly.
  • InteractiveRandom is now used in this function which may cause issues, this has not been tested.
  • This is my first time fully diving into the OpenTTD codebase, and so I might have missed some things / coding practices.
  • Commit log is a bit too messy, tried to squash some of them, but it might still need retouching.

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 touches english.txt or translations? Check the guidelines
  • 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, game_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@JGRennison
Copy link
Owner

The reason for the notes in the code about InteractiveRandom, is that this is not multiplayer safe, and as written this PR will result in desyncs when the feature is used in a multiplayer game.

If I get some time I can look at resolving that and the other listed issues though.

@JGRennison
Copy link
Owner

Actually, looking more closely, this won't work in multiplayer at all, it's not just a desync issue.

@tabytac
Copy link
Author

tabytac commented Sep 7, 2024

Ahh, thank you for the explanation on the reasoning about InteractiveRandom and the desyncs. I didn’t think about it, but it makes sense now.

Can you say why using the Random function instead of InteractiveRandom wouldn't work in this case? Does it also lead to multiplayer issues and desyncs? I’m sure there’s a reason for sticking with InteractiveRandom, but I'm interested in this PR and would like to help if possible.

@JGRennison
Copy link
Owner

InteractiveRandom is not synchronised with the game state, and so will give different results on each client in a network game.
The Random function is not truly random, but is a pseudo-random function which gives the same result for each client in a network game, because the seed/state is included in the savegame from the server, and calls to it must be made in the context of game state updates or network commands which are shared to and run synchronised on all clients.
The scenario editor is single player only so does not need to worry about any of this. It just does the tree operations locally. To work properly in game it'd need to use the command layer, so that the tree operations can be shared/synchronised with the server/all other clients.

@tabytac
Copy link
Author

tabytac commented Sep 8, 2024

Ahhh ok that makes a lot more sense on how each of those work and the limitations. Now a lot of the decisions on what features still remain scenario editor exclusive also make sense since alot of them also "randomly" affect tiles etc.

Thanks for sharing the reason on why this isn't as simple as just enabling the buttons for normal gameplay and actually talking through the technical reasoning behind it!

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.

2 participants