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

libtrx/config: support enforced config settings #1854

Merged
merged 2 commits into from
Nov 10, 2024

Conversation

lahm86
Copy link
Collaborator

@lahm86 lahm86 commented Nov 9, 2024

Resolves #1846.
Resolves #1847.

Checklist

  • I have read the coding conventions
  • I have added a changelog entry about what my pull request accomplishes, or it is an internal change

Description

This allows an enforced object to be defined in the config file, within which any regular config setting can be defined, and the values from here will be enforced in the game. This applies to TR1 and TR2.

Example snippet for TR1X.json5:

{
  "enable_uw_roll" : true,
  "enforced" : {
    "enable_uw_roll" : false
  }
}

Here the initial value is replaced by the one in the enforced object when the game loads. When the game writes back the config, both entries will be preserved, meaning if players want to remove these enforcements they can do so and their original settings will not need to be re-edited.

The config tool will grey out any enforced options, and display a message so the user knows why they cannot be edited.

I think we can remove force_game_modes and force_save_crystals from the gameflow and allow builders to implement it in the same way as above. But probably best handled in its own PR and after review/testing of this one.

Re #1847, this same approach can be used by builders to remove features available only with Lara's animation injection. The alternative to this is testing animation goals, which IMO would result in too many additional code checks.

@lahm86 lahm86 added Feature New functionality TR2 TR1 labels Nov 9, 2024
@lahm86 lahm86 self-assigned this Nov 9, 2024
@lahm86 lahm86 requested review from a team as code owners November 9, 2024 17:33
@lahm86 lahm86 requested review from rr-, walkawayy and aredfan and removed request for a team November 9, 2024 17:33
Copy link

github-actions bot commented Nov 9, 2024

Copy link
Collaborator

@walkawayy walkawayy left a comment

Choose a reason for hiding this comment

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

I think everything looks good and works well!

My one suggestion is to change how the config tool shows the enforced option. Right now, the tool shows the user's version of the option greyed out. However, I think the tool should show the enforced option greyed out. For example, using your example setting from the PR force disables the underwater roll:

  "enforced" : {
    "enable_uw_roll" : false
  }

That enforced option is properly enforced in game so Lara can't roll. But, the config tool's greyed out option makes it seem like the author force enabled the underwater roll as seen here:

image

This is confusing because the player would assume the underwater roll is enforced on. I think the opposite can also occur if the user has an option disabled, but the author force enabled it. The player would think it's off but it's actually always on.

@lahm86 lahm86 force-pushed the issue-1846-allow-enforced-config branch from e10fac0 to 688f385 Compare November 9, 2024 21:08
@lahm86
Copy link
Collaborator Author

lahm86 commented Nov 9, 2024

Pushed an update @walkawayy. Unfortunately, because of the abstraction in how the properties are managed, I couldn't think of a good way to store the enforced value but retain writing the actual value back when needed. But I agree it's clearer to show what's actually in place, so let me know if you think the update is OK. It just shows the raw enforced value along with a message. It's clear there is no UI input as such.

@walkawayy
Copy link
Collaborator

walkawayy commented Nov 9, 2024

Hm it still seems to show my user config option instead of the forced option:

image

I have:

{
  "enable_uw_roll" : true,
  "enforced" : {
    "enable_uw_roll" : false
  }
}

@lahm86
Copy link
Collaborator Author

lahm86 commented Nov 9, 2024

Should show like this - maybe you're running the wrong download?
image

@walkawayy
Copy link
Collaborator

walkawayy commented Nov 9, 2024

Oh oops I didn't notice I had to do a git pull with a rebase. My normal pull failed and I wasn't paying attention. Yes it works perfectly now.

Copy link
Collaborator

@rr- rr- left a comment

Choose a reason for hiding this comment

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

Looks good but we need to describe this mechanism in the gameflow docs for it to be adopted.

@lahm86 lahm86 force-pushed the issue-1846-allow-enforced-config branch from 688f385 to eed62ce Compare November 10, 2024 10:40
@lahm86
Copy link
Collaborator Author

lahm86 commented Nov 10, 2024

Updated GAMEFLOW.md with some details on this. I've used save crystals as an example, although I'd still like to remove the other dedicated options for enforcing that and game modes in a separate PR. I'll update the gameflow doc again there as we mention those forced variables at the top of that document.

@lahm86 lahm86 requested a review from rr- November 10, 2024 10:43
This allows an enforced object to be defined in the config file, within
which any regular config setting can be defined, and the values from
here will be enforced in the game.

Enforced settings are not shown in the config tool, but will be
preserved on write.

Resolves LostArtefacts#1846.
This makes enforced properties read-only inside the config tool, and
adds a banner to the top of the main window to inform the user why some
settings cannot be changed.
@lahm86 lahm86 force-pushed the issue-1846-allow-enforced-config branch from eed62ce to 0b950b0 Compare November 10, 2024 10:45
@lahm86 lahm86 merged commit c0583e1 into LostArtefacts:develop Nov 10, 2024
6 checks passed
@lahm86 lahm86 deleted the issue-1846-allow-enforced-config branch November 10, 2024 17:13
lahm86 added a commit to lahm86/TRX that referenced this pull request Nov 11, 2024
This refactors the enforced config control from LostArtefacts#1854 into the gameflow
and removes the legacy settings for enforcing game modes and save
crystals.

Resolves LostArtefacts#1857.
lahm86 added a commit to lahm86/TRX that referenced this pull request Nov 11, 2024
This refactors the enforced config control from LostArtefacts#1854 into the gameflow
and removes the legacy settings for enforcing game modes and save
crystals.

Resolves LostArtefacts#1857.
lahm86 added a commit to lahm86/TRX that referenced this pull request Nov 11, 2024
This refactors the enforced config control from LostArtefacts#1854 into the gameflow
and removes the legacy settings for enforcing game modes and save
crystals.

Resolves LostArtefacts#1857.
lahm86 added a commit to lahm86/TRX that referenced this pull request Nov 11, 2024
This refactors the enforced config control from LostArtefacts#1854 into the gameflow
and removes the legacy settings for enforcing game modes and save
crystals.

Resolves LostArtefacts#1857.
lahm86 added a commit that referenced this pull request Nov 11, 2024
This refactors the enforced config control from #1854 into the gameflow
and removes the legacy settings for enforcing game modes and save
crystals.

Resolves #1857.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New functionality TR1 TR2
Projects
Archived in project
4 participants