Skip to content

Close #13615: insert required action implementation#13648

Closed
csunday95 wants to merge 8 commits into
OpenRCT2:developfrom
csunday95:develop-refactor-13615-missing-action-impl
Closed

Close #13615: insert required action implementation#13648
csunday95 wants to merge 8 commits into
OpenRCT2:developfrom
csunday95:develop-refactor-13615-missing-action-impl

Conversation

@csunday95

Copy link
Copy Markdown
Contributor
  • add AcceptParameters() where missing to GameAction s exposed in the script engine
  • implement additional deserialization methods as needed in GameAction.h

 - added deserialization Visit() override for MapRange
 - ClearAction now usable via "clearscenery" script engine command
@csunday95

Copy link
Copy Markdown
Contributor Author

intended to replace #13616

 - implement LoadOrQuitAction deserialization
 - implement NetworkModifyGroupAction deserialization
 - implement ParkEntranceRemoveAction deserialization
 - implement SmallScenerySetColourAction
 - added deserialization for ParkSetParameterAction
 - added deserialization for ParkMarketingAction
 - added deserialization for ParkSetDateAction
 - added deserialization for ParkSetLoanAction
 - ParkSetParameterAction
 - ParkSetResearchFundingAction
 - PeepPickupAction
 - PlaceParkEntranceAction
 - PlacePeepSpawnAction
 - PlayerKickAction
 - PlayerSetGroupAction
@duncanspumpkin

Copy link
Copy Markdown
Contributor

Don't open a new PR for the same thing. You can pretty much always recover your branch. By opening a new one we loose all the comments that we made on the previous one.

{
}

void FootpathPlaceFromTrackAction::AcceptParameters(GameActionParameterVisitor& visitor)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove this. This action should ONLY be used by track designs and never by plugins.

{
}

void LoadOrQuitAction::AcceptParameters(GameActionParameterVisitor& visitor)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

debatable if this is safe to perform from a plugin as it will unload the plugin. This may cause unexpected behaviour.

{
}

void PeepPickupAction::AcceptParameters(GameActionParameterVisitor& visitor)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will plugins understand this action. Its not a simple action to use.

@duncanspumpkin

Copy link
Copy Markdown
Contributor

Make sure all of the comments from #12933 are applied as well.

@csunday95

csunday95 commented Dec 26, 2020

Copy link
Copy Markdown
Contributor Author

The actions that I am implementing are based entirely on the list of exposed actions in ScriptEngine.cpp:964. If there are actions we don't want plugins to use, they should be removed there rather than just having a broken implementation. So far it looks like we don't want

  • LoadOrQuitAction - agree this is probably unsafe
  • FootpathPlaceFromTrackAction - doesn't really make sense to expose this

As for the more complex actions (e.g. PeepPickupAction), it may be best to implement a simpler plugin interface if the action is desired. Remaining exposed actions in script engine that are missing implementation include:

  • SetSignNameAction
  • SetSignStyleAction
  • StaffFireAction
  • StaffSetColourAction
  • StaffSetCostumeAction
  • StaffSetOrdersAction
  • StaffSetPatrolAreaAction
  • SurfaceSetStyleAction - probably don't need to expose this?
  • TileModifyAction - possibly a little complex / deep to expose to the script engine?
  • WallSetColourAction
  • WaterLowerAction
  • WaterRaiseAction
  • WaterSetHeightAction

@csunday95

csunday95 commented Dec 26, 2020

Copy link
Copy Markdown
Contributor Author

one comment from #12933 is I think worthy of discussion:

There are a few here that I do not think should be usable by plugins and others that are dangerous to use. A lot of the parameters need some careful thought on their names as we should standardise it as much as possible to prevent issues with later renaming.

A standardized naming scheme for arguments would be nice, as would a document detailing all the exposed callbacks and their arguments. There's also the general concern of the plugin user needing to pass enumeration values, which will work under the current system, but be very arcane and require source code examination until a document is made.

 - Did not add implementation for SurfaceSetStyleAction or
   TileModifyAction, as these likely should not be exposed in the script
   engine
 - Remove FootpathPlaceFromTrackAction - this action should be purely
   internal
 - remove LoadOrQuitAction - doesn't make much sense for a plugin to use
   this and likely causes unsafe plugin unload
 - SurfaceSetStyleAction - complex to use via plugin and unlikely to be
   needed?
 - TileModifyAction - seems like it should be kept internal
@csunday95 csunday95 changed the title fix #13615 by inserting required action implementation Close #13615: insert required action implementation Dec 26, 2020
{ "staffsetpatrolarea", GameCommand::SetStaffPatrol },
{ "surfacesetstyle", GameCommand::ChangeSurfaceStyle },
{ "tilemodify", GameCommand::ModifyTile },
{ "trackdesign", GameCommand::PlaceTrackDesign },

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think for these actions that have been removed from the plugin API, like footpathplacefromtrack, loadorquit and others, their AcceptParameters function should rather be no-ops and actually have a comment there stating why they're intentionally empty, so that people don't think we just forgot to add them a few moments from now.

@Gymnasiast Gymnasiast linked an issue Feb 8, 2021 that may be closed by this pull request
@IntelOrca

Copy link
Copy Markdown
Contributor

@csunday95 are you able to rebase this? It might help if you break it down into separate PRs to reduce the size. Perhaps start with the simpler actions that haven't been commented on (assumption being no one had any concerns with them).

@Gymnasiast

Copy link
Copy Markdown
Member

Closing this as it appears to be abandoned.

Agree with @IntelOrca on the way forward, should OP want to pick it up again.

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.

Some Game Actions have incomplete implementations

5 participants