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

List of API/Compat improvements and changes that could end up being useful #411

Open
SokyranTheDragon opened this issue Jan 2, 2024 · 2 comments
Labels
1.4 Fixes or bugs relating to 1.4 (Not Biotech). enhancement New feature or request.

Comments

@SokyranTheDragon
Copy link
Member

The primary reason for me making this issue is to raise some discussion on stuff we could add or change to either improve compatibility, or extend the API. Personally, many of those would be useful when patching compat for mods. However, not all of them are fully needed or something that would be reasonable to implement (specifically the last one).

@Zetrith @notfood I'd appreciate your thoughts on the list I've prepared. Perhaps some other ideas of things to add to here?

I personally would be able to (somewhat quickly) implement the first 4-5 objects on the list here, as well as the one for accessing faction from IPlayerInfo. I believe it would take a few days at most. However, I'm not exactly sure how to easily access current player's PlayerInfo object (tried to look through code, but couldn't find a simple way to do it besides relying on player's username). As for the other positions on this list, I could look into them - but currently don't see them as a priority.

Potential API/Compat improvements and changes that could be implemented:

  • Custom handlers for syncing Verb
    • Perhaps this could be done by syncing their parent in a similar way to IThingHolder? (a list of supported types, and synced using WriteWithImpl<T>/ReadWithImpl<T>)
    • Not the most common thing to happen in mods that need compat, but it does happen (currently happening with XVI-MECHFRAME)
  • Ability to easily add additional entries to supportedThingHolders to support custom IThingHolder implementations
    • There's currently 1 mod in MP Compat that modifies the list of supported thing holders (Vehicle Framework)
    • There's another mod that could have use this (PokéWorld), but was done in a different way
  • Change the VerbOwnerType handler from Pawn to Thing for greater compatibility
  • Ability to exclude specific designators from being automatically synced
    • We've got Designator_MechControlGroup in vanilla that doesn't require syncing, and we already exclude it
    • Currently would simplify the patch for AllowTool
    • This would require changing DesignatorFinalizePatch to not cancel execution if the designator is not synced
    • Could likely be registered by a method call, as well as an attribute on the designator itself
  • A simple way to access and manipulate the 'true' Selector and WorldSelector selected objects when executing commands
    • Currently without using reflection, it's basically impossible to check selected objects or select new ones in a synced context
    • Even when using reflection, it's not always possible - it's only possible to access Selector it if it was temporarily replaced by AsyncTimeComp, but not DebugSync or SyncUtil, as those don't store it in a temporary field
    • This currently prevents situations where an object is despawned and a new one replacing it spawned, while also keeping the new object selected (keeping the illusion that it's only a single object)
  • A way to access the current player's IPlayerInfo
    • Currently, this can be achieved by iterating over all players and comparing MP.PlayerName with IPlayerInfo.Username
  • Have the current player's IPlayerInfo actually list the selected objects
    • The current player will always have the list empty
  • Expand IPlayerInfo to be able to lookup specific player's faction
    • Perhaps some other data that would be useful to include here?
  • Ability to push/pop current player faction for situations where it's needed?
    • Honestly, haven't really looked into mod compatibility with multifaction, so I'm unsure of how useful this would be
  • Easy ability to check ticks for specific context (world/specific map) with async time
    • In a couple of situations, I've come across the need to do something based on ticks - however, it was using the world time instead of map time, causing minor issues
    • Could be situationally useful to control time on specific maps
  • This one would be quite a bit of a stretch, but - ability to easily modify how some objects are synced
    • Primary use would be if a mod does something weird, like how Gestalt Engine/Reinforced Mechanoids 2 has a "dummy" pawn tied to a building, which MP fails to sync
    • Still, as I said, this is not something that we'll do and likely not something that should end up in API - especially since it doesn't come up often - but I thought I'd throw it in anyway
@SokyranTheDragon SokyranTheDragon added enhancement New feature or request. 1.4 Fixes or bugs relating to 1.4 (Not Biotech). labels Jan 2, 2024
@notfood
Copy link
Member

notfood commented Jan 3, 2024

  • Verb syncing is very annoying because finding the parent is a challenge when you are writing sync workers, if you can find a better way, it'd be very welcomed.
  • Custom IThingHolder is very very rare, you've got to weight how much you want to add to the API and how much use it will get. Using Harmony to get there isn't a bad thing.
  • VerbOwnerType from Pawn to Thing sounds good
  • Exclude specific designators sounds good, maybe some function to cancel it after an evaluation call instead of blindly cancelling everything.
  • Selector and WorldSelector selected objects, no opinion on this one. I don't remember dealing with it.
  • IPlayerInfo API sounds good. A set of methods to control Player state would come along nicely to implement things like capture the flag or "destroy thing to destroy player"
  • Multifaction is a mystery to me right now.

SokyranTheDragon added a commit to SokyranTheDragon/Multiplayer that referenced this issue Jan 3, 2024
One of the changes I've proposed in rwmt#411, without any API implementation.

List of changes:
- `VerbOwnerType` enum was removed and replaced by `supportedVerbOwnerTypes` array
- The array includes `typeof(Thing)` in stead of `typeof(Pawn)` for increased compatibility
- `IVerbOwner` sync worker entry was added
- `Verb` sync worker entry was modified to sync the owner as `IVerbOwner`

Those changes should result in greater compatibility, as new supported `IVerbOwner` types can now be added to the array and synced using their own sync workers.

This should also end up simplifying `Verb` sync worker going forward, as we won't have to expand it anymore in the future - only the array of supported types.

Things that I did not include, but we may want to potentially consider:
- Add more vanilla types to the list of supported verb owners, which could include:
  - `HediffComp` (specifically for `HediffComp_VerbGiver`) - however, in vanilla RW they don't have gizmos, but a mod could add a comp that adds one
  - `Pawn_MeleeVerbs_TerrainSource` - likely will never gizmos, probably will be completely pointless to include
  - `Pawn_NativeVerbs` - same as above
- Automatically including all subtypes of `IVerbOwner` which have an explicit sync worker
  - Would simplify mod compat, as mods would (likely) never need to modify the list of supported verb owners
  - Could have potentially unintended consequences?
@SokyranTheDragon
Copy link
Member Author

I've dropped a PR (#412) that addresses the first and third points I've made, as well as a mod compat that requires work to fully work (rwmt/Multiplayer-Compatibility#400). Currently stuff can be supported the same way custom IThingHolders can.



Exclude specific designators sounds good, maybe some function to cancel it after an evaluation call instead of blindly cancelling everything.

I suppose a Dictionary<Type, Func<Designator,bool>> (or a custom delegate) could work for this?

This would slightly complicate it a bit. We'd have to check for it in the prefixes for DesignateSingleCell/DesignateMultiCell/DesignateThing, and depending on the result (namely if we let it run without syncing) we'd have to also use the result in DesignatorFinalizePatch, as it may not fully work otherwise. Still, sounds feasible.



Selector and WorldSelector selected objects, no opinion on this one. I don't remember dealing with it.

I just did encounter it, specifically here: https://github.com/rwmt/Multiplayer-Compatibility/blob/4d19b02ccd047ba5944b0fa1aff019f87596be62/Source/Mods/XVIMechframe.cs#L381-L398

I've also encountered this in the past, but ignored it back then. There would be some use at the very least. Also, the current implementation I've linked may not always work - depends on what's being synced, and it will never work for stuff selected in the world map.

Perhaps an option for sync methods to not replace the selectors with empty lists, in situations where it's needed? Would fully remove the need for the patch I made there, as the mod itself would select the proper objects as needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.4 Fixes or bugs relating to 1.4 (Not Biotech). enhancement New feature or request.
Projects
None yet
Development

No branches or pull requests

2 participants