Misc. singleplayer mission additions / improvements #1145
Conversation
|
Nightly build for this pull request:
This comment is automatic and is meant to allow guests to get latest nightly builds for this pull request without registering. It is updated on every successful build. |
Starkku
force-pushed
the
feature/campaign-improvements
branch
2 times, most recently
from
aa2e65c
to
facb83d
Compare
|
Starkku
force-pushed
the
feature/campaign-improvements
branch
from
facb83d
to
9cc4668
Compare
|
Apart from the comments LGTM, looks mergeable. |
Starkku
force-pushed
the
feature/campaign-improvements
branch
from
9cc4668
to
604687c
Compare
WalkthroughThe recent updates enhance the singleplayer experience by introducing a variety of new features and improvements. These include expanded AI control options, customizable mission briefings, and scenario-specific settings. The changes also touch on visual aspects like loading screen details and grid transparency. A new source file has been added to manage hooks, while existing code has been refined to support these new functionalities. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (10)
- CREDITS.md (1 hunks)
- Phobos.vcxproj (1 hunks)
- docs/AI-Scripting-and-Mapping.md (1 hunks)
- docs/Whats-New.md (1 hunks)
- src/Ext/Scenario/Body.cpp (3 hunks)
- src/Ext/Scenario/Body.h (1 hunks)
- src/Ext/Scenario/Hooks.cpp (1 hunks)
- src/Ext/Side/Body.cpp (2 hunks)
- src/Ext/Side/Body.h (2 hunks)
- src/Misc/Hooks.UI.cpp (2 hunks)
Additional comments: 19
src/Ext/Scenario/Hooks.cpp (1)
- 5-26: The hook
ReadScenario_LoadingScreensreads various INI settings related to loading screens and briefing locations. The logic correctly reads individual settings from the scenario name section and falls back to defaults specified in theDefaultssection if not present. The use ofReadIntegerandReadStringwith appropriate fallbacks is correct. The code is clean, follows good practices, and there are no apparent issues with correctness, performance, or maintainability.src/Ext/Scenario/Body.h (2)
- 28-29: The addition of
ShowBriefingandBriefingThemeto theExtDataclass is consistent with the PR's objectives to allow customization of the briefing screen. The default values offalseforShowBriefingand-1forBriefingThemeare sensible defaults indicating disabled state and no theme selected, respectively.- 35-36: The constructor of
ExtDatainitializes the new member variablesShowBriefingandBriefingThemecorrectly. This ensures that new scenarios will have these properties set to their default values if not specified otherwise.src/Ext/Side/Body.h (2)
- 36-36: The
BriefingThememember variable is added to theSideExtclass with a default value of-1, which is consistent with the PR's objectives for allowing sides to have a default briefing theme. The use ofValueable<int>is appropriate for a property that can be set in the INI files.- 56-56: The constructor of
ExtDatainitializes the newBriefingThememember variable to-1, which is a sensible default indicating no theme selected. This is consistent with the rest of the codebase where-1typically indicates an unset or default state.src/Ext/Side/Body.cpp (2)
- 41-41: The
LoadFromINIFilemethod has been updated to read theBriefingThemeproperty from the INI file. The use ofReadThemeis appropriate for reading theme indices. The code is straightforward and follows the existing pattern for reading properties from INI files.- 68-68: The
Serializemethod has been updated to include theBriefingThemeproperty. This ensures that the property is correctly serialized and deserialized, which is necessary for saving and loading game states.src/Ext/Scenario/Body.cpp (2)
- 107-130: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [101-126]
The
LoadFromINIFilemethod inScenarioExt::ExtDatahas been updated to read new settings related to scenario rankings and briefing settings from themissionmd.inifile. The code correctly reads the par times, messages, and briefing settings, with a fallback to the scenario's INI section if not found inmissionmd.ini. The use ofReadBoolandReadThemeis consistent with the types of the properties being read.
- 138-139: The
Serializemethod has been updated to include theShowBriefingandBriefingThemeproperties. This ensures that these properties are correctly handled during the serialization process, which is important for saving and loading game states.src/Misc/Hooks.UI.cpp (3)
- 226-259: The hook at
0x683E41has been updated to check theShowBriefingflag and play the briefing theme if set. The logic correctly checks if the briefing should be shown and plays the theme based on theBriefingThemeproperty. The use of a temporary namespaceBriefingTempto store theShowBriefingstate is a good practice to avoid polluting the global namespace.- 261-286: The hook at
0x48CE85is responsible for showing the briefing dialog and playing the scenario theme. The code correctly resumes the session, checks if the briefing should be shown, and plays the appropriate theme. The logic is straightforward and follows the intended behavior described in the PR objectives.- 288-297: The hook at
0x683F66skips redrawing the screen if the briefing screen is about to be shown. This is a performance optimization to avoid unnecessary screen redraws. The code is simple and effective.CREDITS.md (1)
- 218-218: The addition of "Misc. singleplayer mission improvements" to the credits file under Starkku's contributions is consistent with the changes made in this PR. It's important to acknowledge contributors for their work, and this change does so appropriately.
Phobos.vcxproj (1)
- 42-42: The addition of
src\Ext\Scenario\Hooks.cppto the project file is noted. Ensure that this new source file is accompanied by a corresponding header file if needed, and that it adheres to the project's structure and naming conventions.docs/AI-Scripting-and-Mapping.md (4)
- 19-24: The boolean values for
RepairBaseNodeshave been changed fromnotofalse. This is a consistency improvement, aligning with standard boolean values.- 46-51: The option
MCVRedeployshas been changed fromnotofalse, which is a consistency improvement.- 56-67: The documentation now includes information on setting par times and related string labels in
missionmd.ini. This is a new feature that allows for more detailed configuration of singleplayer missions.- 72-86: The documentation has been updated to include the
ShowBriefingandBriefingThemesettings, which allow for the display of a briefing dialog and customization of the briefing theme music. This enhances the singleplayer campaign mission startup experience.docs/Whats-New.md (1)
- 353-355: The additions to the changelog for the upcoming version are clear and consistent with the PR objectives. The features described in lines 353 to 355 match the PR's aim to enhance singleplayer missions.
| - It is now possible to set defaults for singleplayer map loading screen briefing pixel offsets and the loading screen images and palette that are used if there are no values defined for the map itself. | ||
| - Note that despite the key name being `DefaultLS800BkgdPal`, this applies to both shapes just like the original scenario-specific `LS800BkgdPal` does. | ||
|
|
||
| - In `missionmd.ini`: | ||
| ```ini | ||
| [Defaults] | ||
| DefaultLS640BriefLocX=0 ; integer | ||
| DefaultLS640BriefLocY=0 ; integer | ||
| DefaultLS800BriefLocX=0 ; integer | ||
| DefaultLS800BriefLocY=0 ; integer | ||
| DefaultLS640BkgdName= ; filename - including the .shp extension. | ||
| DefaultLS800BkgdName ; filename - including the .shp extension. | ||
| DefaultLS800BkgdPal= ; filename - including the .pal extension |
There was a problem hiding this comment.
The documentation has been updated to reflect the ability to set default loading screen and briefing offsets, images, and palettes. The note on line 30 about the misleading key name DefaultLS800BkgdPal is important for clarity. However, there is a missing = sign on line 40 for DefaultLS800BkgdName.
- DefaultLS800BkgdName ; filename - including the .shp extension.
+ DefaultLS800BkgdName= ; filename - including the .shp extension.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| - It is now possible to set defaults for singleplayer map loading screen briefing pixel offsets and the loading screen images and palette that are used if there are no values defined for the map itself. | |
| - Note that despite the key name being `DefaultLS800BkgdPal`, this applies to both shapes just like the original scenario-specific `LS800BkgdPal` does. | |
| - In `missionmd.ini`: | |
| ```ini | |
| [Defaults] | |
| DefaultLS640BriefLocX=0 ; integer | |
| DefaultLS640BriefLocY=0 ; integer | |
| DefaultLS800BriefLocX=0 ; integer | |
| DefaultLS800BriefLocY=0 ; integer | |
| DefaultLS640BkgdName= ; filename - including the .shp extension. | |
| DefaultLS800BkgdName ; filename - including the .shp extension. | |
| DefaultLS800BkgdPal= ; filename - including the .pal extension | |
| [Defaults] | |
| DefaultLS640BriefLocX=0 ; integer | |
| DefaultLS640BriefLocY=0 ; integer | |
| DefaultLS800BriefLocX=0 ; integer | |
| DefaultLS800BriefLocY=0 ; integer | |
| DefaultLS640BkgdName= ; filename - including the .shp extension. | |
| DefaultLS800BkgdName= ; filename - including the .shp extension. | |
| DefaultLS800BkgdPal= ; filename - including the .pal extension |
There was a problem hiding this comment.
@Starkku seems you missed that one
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
idk if I'm using this the correct way, but this doesn't seem to work as expected at all. |
missionmd.iniDefault loading screen and briefing offsets
It is now possible to set defaults for singleplayer map loading screen briefing pixel offsets and the loading screen images and palette that are used if there are no values defined for the map itself.
In
missionmd.ini:Set par times and related string labels in missionmd.ini
[Ranking]section of the map file itself. These can now also be set in the map file's section inmissionmd.ini, taking precedence over the map file's settings but defaulting to them if not set.In
missionmd.ini:Show briefing dialog on startup
ShowBriefingto true in map file's[Basic]section, or in the map file's section inmissionmd.ini(latter takes precedence over former if available).BriefingTheme(In order of precedence from highest to lowest:missionmd.ini, map file, side entry inrulesmd.ini) can be used to define a custom theme to play on this briefing screen. If not set, the loading screen theme will keep playing until the scenario starts properly.In
missionmd.ini:In map file:
In
rulesmd.iniSummary by CodeRabbit
New Features
Documentation
Refactor
Bug Fixes