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

Changed verb syncing to work closer like IThingHolder #412

Conversation

SokyranTheDragon
Copy link
Member

One of the changes I've proposed in #411, but without any API implementation.

List of changes:

  • VerbOwnerType enum was removed and replaced by supportedVerbOwnerTypes array
  • The array includes typeof(Thing) instead 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 due to HediffComp_VerbGiver) - in vanilla RW they don't have gizmos, but a mod could add a custom hediff comp that adds them
    • Pawn_MeleeVerbs_TerrainSource - likely will never get gizmos, probably will be completely pointless to include
    • Pawn_NativeVerbs - same as above
  • Automatically including all subtypes of IVerbOwner which have a(n 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?

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?
@Zetrith
Copy link
Member

Zetrith commented Jan 6, 2024

I think IVerbOwner and ISelectable can just be handled like the other interfaces. The worst that can happen is an error message when syncing an unsupported implementation.

I've placed `IVerbOwner` sync worker in the `Interfaces` region, which I've placed at the very bottom. This is due to issues with syncing `IVerbOwner` - for example, trying to sync a `Pawn` caused it to be synced as `IVerbOwner`, which caused it to be recursively synced as `IVerbOwner` until the game crashed.

Placing it lower fixed the issue.
@SokyranTheDragon
Copy link
Member Author

I've attempted to sync IVerbOwner like other interfaces, but sadly - it doesn't look like it'll work without other changes. I thought I managed to fix it, but seems like it's not the case, so I reverted the commit.

The issue is trying to sync (for example) a Pawn, which causes it to be synced as IVerbOwner. IVerbOwner then tries to sync the Pawn as IVerbOwner, which tries to... You get the idea. It tries to recursively sync Pawn as IVerbOwner over and over until the game crashes.

@Zetrith Zetrith mentioned this pull request Mar 15, 2024
5 tasks
@Zetrith
Copy link
Member

Zetrith commented Apr 10, 2024

This should be entirely addressed by b5a01c3

@Zetrith Zetrith closed this Apr 16, 2024
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).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants