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

Psychic ritual syncing #482

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

Conversation

SokyranTheDragon
Copy link
Member

I've decided to look into this over the past 2-3 days and have mostly finished it, however there's still a few things that are needed (so it's currently a draft). Things still needing work:

  • Modify ritual prepatches to also work with psychic rituals, so assigning roles will be synced
  • Add localization for MpPsychicRitualSession

Info on psychic ritual session, dialog, and patches I've included:

  • Heavily based on ritual patches (started by copy-pasting session/proxy code related to them)
  • I've not used "PsychicRitualData" like how it's done with "RitualData" for normal rituals, as I've decided there wasn't as much data to handle
    • If needed, I can easily change this
  • I've synced the start call by overriding the "Start" method in ritual proxy
    • This can also be applied to normal rituals to remove a harmony patch
    • In case of normal rituals, we could also include a confirmation dialog to match vanilla behaviour
  • Syncing of creating session was done by patching PsychicRitualGizmo.InitializePsychicRitual
    • As opposed to normal rituals that can be started in multiple places, this is the only ___location those can be created from
  • I believe everything else should have comments, match behaviour of other session, or be self-explanatory
    • If explanation for anything else is needed, let me know, and I'll include it in code comments or on GitHub

A slight side note about dialogs with sessions (and styling station dialog) - perhaps we should make a single harmony patch to Widgets.ButtonTextWorker with a generalized way to use it? As it stands right now, we basically make two patches per session (prefix and postfix). Having a more generalized way to handle it may be useful in the future.

I've decided to look into this over the past 2-3 days and have mostly finished it, however there's still a few things that are needed (so it's currently a draft). Things still needing work:
- Modify ritual prepatches to also work with psychic rituals, so assigning roles will be synced
- Add localization for `MpPsychicRitualSession`

Info on psychic ritual patches I've included:
- Heavily based on ritual patches (started by copy-pasting the code related to them)
- I've not used "PsychicRitualData" like how it's done with "RitualData" for normal rituals, as I've decided there wasn't as much data to handle
- I've synced the start call by overriding the "Start" method in ritual proxy
  - This can also be applied to normal rituals to remove a harmony patch
  - In case of normal rituals, we could also include a confirmation dialog to match vanilla behaviour
- Syncing of creating session was done by patching `PsychicRitualGizmo.InitializePsychicRitual`
  - As opposed to normal rituals that can be started in multiple places, this is the only ___location those can be created from
- I believe everything else should have comments, match behaviour of other session, or be self-explanatory
  - If explanation for anything else is needed, let me know, and I'll include it in code comments or on GitHub

A slight side note about dialogs with sessions (and styling station dialog) - perhaps we should make a single harmony patch to `Widgets.ButtonTextWorker` with a generalized way to use it? As it stands right now, we basically make two patches per session (prefix and postfix). Having a more generalized way to handle it may be useful in the future.
@SokyranTheDragon SokyranTheDragon added the anomaly Fix or bugs relating to Anomaly (Not 1.5) label Jul 14, 2024
@SokyranTheDragon
Copy link
Member Author

In case someone skimmed through the contents of the main message - this is a draft because we need to modify ritual prepatches to also work with psychic rituals, so assigning roles will be synced.

First, a psychic ritual dialog desync was fixed by seeding the RNG when it may end up being used.

Second (and more importantly), the psychic rituals were synced further. I hope that everything should work now, unless I've missed something.

I have to admit - syncing was done it a rather crude and simple way. I've changed the code to apply the same prepatches for both rituals and psychic rituals (which means 2 patches are applied to those methods now).

This approach could be improved by changing `PrepatcherPrefix` to accept all subtypes (rather than just having an exact type match). This will reduce the amount of prepatches needed, but (without further changes) won't fully eliminate them - 2 of the patches rely on transforming the arguments, and I could not think of a clear and simple way to handle them.

In any case, I've extracted the argument transformer writer and reader used there into their own methods, and I've made them generic methods that will handle both normal and psychic rituals.

A final addition was adding a sync worker for `PawnPsychicRitualRoleSelectionWidget`, as it's used by the synced methods.
@SokyranTheDragon
Copy link
Member Author

I've made a new commit with a few changes/additions.

First, a psychic ritual dialog desync was fixed by seeding the RNG when it may end up being used.

Second (and more importantly), the psychic rituals were synced further. I hope that everything should work now, unless I've missed something.

I have to admit - syncing was done it a rather crude and simple way. I've changed the code to apply the same prepatches for both rituals and psychic rituals (which means 2 patches are applied to those methods now).

This approach could be improved by changing PrepatcherPrefix to accept all subtypes (rather than just having an exact type match). This will reduce the amount of prepatches needed, but (without further changes) won't fully eliminate them - 2 of the patches rely on transforming the arguments, and I could not think of a clear and simple way to handle them. @Zetrith let me know if you'd like me to change this so the prefix will apply to all rituals (ignoring the 2 methods that need transforming arguments).

In any case, I've extracted the argument transformer writer and reader into their own methods, and I've made them generic methods that will handle both normal and psychic rituals.

A final addition was adding a sync worker for PawnPsychicRitualRoleSelectionWidget, as it's used by the synced methods.

@SokyranTheDragon SokyranTheDragon marked this pull request as ready for review August 1, 2024 20:40
@SokyranTheDragon
Copy link
Member Author

I've also decided to mark this PR as ready for review (rather than a draft), as I believe it should be mostly complete (again, ignoring some minor changes to prepatches).

SokyranTheDragon added a commit to SokyranTheDragon/Multiplayer-Locale that referenced this pull request Aug 7, 2024
@SokyranTheDragon
Copy link
Member Author

One thing I just had - should sessions for normal and psychic rituals be mutually exclusive? Thinking about it, it seems like it may be safer to do it that way, as this would avoid any issues where pawns are assigned to both a normal and psychic ritual at the same time.

Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
anomaly Fix or bugs relating to Anomaly (Not 1.5)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant