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.

Sign up for GitHub

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

Jump to bottom

Expose more functions to game scripts #10411

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Expose more functions to game scripts #10411

wants to merge 6 commits into from

Conversation

SamuXarick
Copy link
Contributor

@SamuXarick SamuXarick commented Jan 25, 2023 •
edited

Motivation / Problem

  1. A few functions could work without needing the GS to be in CompanyMode. These have been changed to allow it:
  • GSGroup::IsValidGroup
  • GSGroup::GetNumEngines
  • GSGroup::GetNumVehicles
  • GSGroup::GetEngineReplacement
  • GSGroupList
  • GSVehicleList_Group
  • GSVehicleList_DefaultGroup
  1. One special method is added and only exposed for Game Scripts to complement for the changes above:
  • GSGroup::GetOwner
  1. Expose more events to Game Scripts. They and all other events have been templated as requested:
  • GSEventEnginePreview
  • GSEventCompanyAskMerger
  • GSEventVehicleLost
  • GSEventVehicleWaitingInDepot
  • GSEventVehicleUnprofitable
  • GSEventEngineAvailable
  • GSEventDisasterZeppelinerCrashed
  • GSEventDisasterZeppelinerCleared
  • GSEventAircraftDestTooFar
  • GSEventVehicleAutoReplaced

Description

  • ScriptGroup::IsValidGroup, ScriptGroupList, ScriptVehicleList_Group and ScriptVehicleList_DefaultGroup lets GS to get groups of any company without being in the company.

  • ScriptGroup::GetOwner didn't exist before. It was added and is only accessible for the GS.

Limitations

  • ScriptGroup::GetNumEngines, ScriptGroup::GetNumVehicles and ScriptGroup::GetEngineReplacement have extra conditions to make it less restrictive to use when in deity mode. If the group exists and is not GROUP_DEFAULT and not GROUP_ALL, then the company of that group could be retrieved from the group->owner itself, without the need to be in company scope, but since those functions can also be supplied with GROUP_DEFAULT and GROUP_ALL as parameters, you need to be in a valid company scope mode for those.

  • ScriptEventEngineAvailable may happen to not be broadcasted for AIs, only to GS, because a GS can also deal with non-AI companies.

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, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@SamuXarick SamuXarick mentioned this pull request Jan 25, 2023
Closed
@SamuXarick

This comment was marked as resolved.

Sign in to view
@SamuXarick SamuXarick force-pushed the expose-more-functions-to-GSs branch 2 times, most recently from e45a708 to 3cf24b9 Compare January 26, 2023 15:42
@2TallTyler
Copy link
Member

Is this ready for review? Andy asked for #10381 to be merged, but it's missing the changelog and company mode preconditions that you added here, so it's better to merge this and close Andy's.

@SamuXarick SamuXarick marked this pull request as ready for review February 5, 2023 17:57
@SamuXarick
Copy link
Contributor Author

Is this ready for review? Andy asked for #10381 to be merged, but it's missing the changelog and company mode preconditions that you added here, so it's better to merge this and close Andy's.

It kind of is, I'm marking it ready for review. What I'd still want to do was to make some group functions precondition less restrictive when in deity mode, those that only query some information detail from a group.
If the group exists and is not GROUP_DEFAULT and not GROUP_ALL, then the company of that group could be retrieved from the group->owner itself, without the need to be in company scope, but since those functions can also be supplied with GROUP_DEFAULT and GROUP_ALL as parameters, they need to be in company scope for those.

rubidium42
rubidium42 reviewed Feb 9, 2023
View reviewed changes
src/script/api/script_company.cpp Outdated Show resolved Hide resolved
src/script/api/script_group.cpp Outdated Show resolved Hide resolved
src/script/api/script_group.cpp Outdated
/* static */ ScriptCompany::CompanyID ScriptGroup::GetOwner(GroupID group_id) {
if (!IsValidGroup(group_id)) return ScriptCompany::COMPANY_INVALID;

return static_cast<ScriptCompany::CompanyID>((int)::Group::Get(group_id)->owner);
Copy link
Contributor

Choose a reason for hiding this comment

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

The cast via (int) seems weird. Is the (int) really needed? Because then why not use just (ScriptCompany::CompanyID)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I merely copied how it was done on ScriptVehicle::GetOwner. It looks like that, too. Git blame didn't show anything suspicious about it. The first time it appeared was already like this.

Copy link
Contributor

Choose a reason for hiding this comment

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

That it exists somewhere else does not necessarily mean that it is correct. Just to prove a point, yesterday I got some review comments on code that I copied and I amended my code and the code where I copied it from.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done for this case, not for the other GetOwner.

src/script/api/script_group.cpp Outdated Show resolved Hide resolved
src/script/api/script_group.cpp Outdated Show resolved Hide resolved
src/script/api/script_group.cpp Outdated Show resolved Hide resolved
src/script/api/script_group.cpp Outdated Show resolved Hide resolved
src/script/api/script_event_types.hpp Outdated Show resolved Hide resolved
src/autoreplace_cmd.cpp Outdated Show resolved Hide resolved
src/script/api/script_company.cpp Outdated Show resolved Hide resolved
@SamuXarick SamuXarick mentioned this pull request Feb 9, 2023
Merged
@SamuXarick SamuXarick force-pushed the expose-more-functions-to-GSs branch 3 times, most recently from d928b90 to ff23122 Compare February 11, 2023 12:04
@SamuXarick SamuXarick force-pushed the expose-more-functions-to-GSs branch 7 times, most recently from f2f1f4d to 274833b Compare March 8, 2023 23:19
rubidium42
rubidium42 reviewed Mar 9, 2023
View reviewed changes
src/script/api/script_company.hpp Outdated Show resolved Hide resolved
src/script/api/script_group.cpp Outdated
/* static */ ScriptCompany::CompanyID ScriptGroup::GetOwner(GroupID group_id) {
if (!IsValidGroup(group_id)) return ScriptCompany::COMPANY_INVALID;

return static_cast<ScriptCompany::CompanyID>((int)::Group::Get(group_id)->owner);
Copy link
Contributor

Choose a reason for hiding this comment

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

That it exists somewhere else does not necessarily mean that it is correct. Just to prove a point, yesterday I got some review comments on code that I copied and I amended my code and the code where I copied it from.

src/script/api/script_group.cpp
@@ -110,20 +117,27 @@

/* static */ SQInteger ScriptGroup::GetNumEngines(GroupID group_id, EngineID engine_id)
{
EnforceCompanyModeValid(-1);
if (!IsValidGroup(group_id) && group_id != GROUP_DEFAULT && group_id != GROUP_ALL) return -1;
EnforceDeityOrCompanyModeValid(-1);
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the rationale for all this complicated logic, over just requiring company mode for all ScriptGroup calls?
Why is it needed that a GS can iterate over almost, but not all, groups?

Especially because to reach all functionality of this function, I still need to switch to company mode. I really do not see a reason for a GS to iterate (almost) all the groups, I actually do not see much reason for a GS to access groups but I can agree that if adding it takes only five characters there is little harm. However, making the API complicated for something that will barely get used seems overkill.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The rationale, if it is querying information without modifying anything in the company, then it should be done without being in the company. Too bad that for this and 2 other functions to retrieve information for GROUP_DEFAULT and GROUP_ALL, a specific company must be provided. The only way for that is to EnforceCompanyModeValid. The other groups are company owned, which makes it possible to query this information without being in the company.

Copy link
Contributor

Choose a reason for hiding this comment

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

You say that if you don't modify the company you should not need to be in the company, but then still require the GS to be in the company to get the complete picture.

And to be advocate's devil: why shouldn't this function, in deity mode, not return the total number of engines of all companies in the default or all group? Similarly, shouldn't ScriptVehicleList_DefaultGroup contain all vehicles that are not in a group if you're deity? After all, in deity mode basically all the other lists are the combined result of all companies.

That way you do not need to construct complicated preconditions, or add new precondition check functions. And the GS does not need to iterate through all the companies if it wants the combined information.

src/script/script.hpp Outdated Show resolved Hide resolved
src/script/script.hpp Outdated Show resolved Hide resolved
@SamuXarick SamuXarick force-pushed the expose-more-functions-to-GSs branch 7 times, most recently from ca10874 to c271008 Compare March 10, 2023 14:27
rubidium42
rubidium42 reviewed Mar 10, 2023
View reviewed changes
src/engine.cpp Outdated Show resolved Hide resolved
@rubidium42 rubidium42 mentioned this pull request Apr 12, 2023
Merged
@2TallTyler 2TallTyler added the work: needs more work This Pull Request needs more work before it can be accepted label Apr 21, 2023
@SamuXarick SamuXarick force-pushed the expose-more-functions-to-GSs branch 2 times, most recently from 53fe240 to 4bcd137 Compare January 23, 2024 18:55
@SamuXarick SamuXarick force-pushed the expose-more-functions-to-GSs branch from ee0ca3f to 4b97736 Compare March 3, 2024 10:42
@SamuXarick
Add: ScriptGroup::GetOwner for Game Scripts
25caf34
@SamuXarick
Change: Allow a few more GS functions to work without needing to be i…
ca90b69 
…n CompanyMode

ScriptGroup::IsValidGroup, ScriptGroupList, ScriptVehicleList_Group and ScriptVehicleList_DefaultGroup lets GS to get groups of any company without being in the company.

ScriptGroup::GetNumEngines, ScriptGroup::GetNumVehicles and ScriptGroup::GetEngineReplacement have extra conditions to make it less restrictive to use when in deity mode. If the group exists and is not GROUP_DEFAULT and not GROUP_ALL, then the company of that group could be retrieved from the group->owner itself, without the need to be in company scope, but since those functions can also be supplied with GROUP_DEFAULT and GROUP_ALL as parameters, you need to be in a valid company scope mode for those.
@SamuXarick
Codechange: Template NewEvent, BroadcastEvent and BroadcastEventExcep…
323860f 
…tForCompany
@SamuXarick
Change: Allow GS access to more ScriptEvent functions
c404e0e
@SamuXarick
Change: Allow GS access to ScriptEventCompanyAskMerger
6d2e2ea
@SamuXarick
Change: Allow GS access to ScriptEventEnginePreview
5651263
@SamuXarick SamuXarick force-pushed the expose-more-functions-to-GSs branch from 4b97736 to 5651263 Compare March 9, 2024 11:55
@TrueBrain TrueBrain added the needs review: Script API Review requested from GS/AI expert label Mar 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rubidium42 rubidium42 rubidium42 left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
needs review: Script API Review requested from GS/AI expert work: needs more work This Pull Request needs more work before it can be accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@SamuXarick @2TallTyler @rubidium42 @TrueBrain