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

Session rework #401

Closed
wants to merge 33 commits into from
Closed

Conversation

SokyranTheDragon
Copy link
Member

@SokyranTheDragon SokyranTheDragon commented Nov 6, 2023

Changes/additions:

  • Added SessionManager
  • The SessionManager will handle storing and processing all the sessions
  • There's one SessionsManager for MultiplayerWorldComp, as well as one for each MultiplayerMapComp
  • Sessions are now assigned an ID based on their own ID numbers, instead of using Thing IDs for that
  • Instead of sessions self-assigning their own IDs, the IDs are now assigned by SessionManager
  • All existing sessions were reworked to use the new system
  • A Session class has been added which should be used by mods implementing their own sessions (should we remove ISession interface and just use Session class?)
  • The Session class includes a basic implementation for handling SessionId
  • The Session class includes an empty virtual implementation for some methods which not all mods need to implement
  • The Session class exists (as opposed to just using ISession) as a way to make it easy to expand it without requiring the mods to update - the Session class will contain the basic implementation for all new methods in the future (should we do it with other interfaces?)
  • Session, ExposableSession, and SemiPersistentSession were created instead of using interfaces to make it simpler to expand them in the future without requiring mods to update (by adding empty virtual methods as needed). Their respective interfaces were removed as they now served no purpose.
  • The Session class includes a static method SwitchToMapOrWorld for convenience
  • Pause locks were reworked to use the new session system, will only be used if there's any pause lock registered
  • SyncSessionWithTransferablesMarker.DrawnSessionWithTransferables has been included, somewhat based on how ThingFilterMarkers.DrawnThingFilter is implemented, used for syncing Trasferables in ISessionWithTransferables sessions
  • Unique sessions, as well as sessions with other restrictions, are handled by ISessionWithCreationRestrictions - they sadly need to be initialized to work (static members in interfaces seem to not be available in .NET used by RimWorld)

Some things that could be done:

  • Include a cached field for all sessions when needing a specific session (caravan forming, caravan splitting, etc.), similarly to the list of trade sessions. Currently all but trading sessions are accessed through session manager.
  • When adding a Session to the manager, only the session itself (if it's ISessionWithCreationRestrictions) checks if it should be added - any sessions already existing don't check against the one being added. This could be changed to work both ways.

Things left to do:

  • Finish documentation for ISessionManager
  • Add methods for MultiplayerAPIBridge that will be included in updated MP API for accessing SessionManager
  • ISessionWithTransferables will require proper <see cref=""/> tags once included in the MP API.

Things that will need to be moved to MP API:

  • Session class - possibly could use a rename
  • ISessionManager interface
  • All session related interfaces - the only exception could possibly be IPausingWithDialog, as it seems to be right now
  • Should ISwitchToMap be included (and potentially expanded)?

Testing:

  • I've tried to test every single existing sessions, including starting one, operating on it, and then joining with a second client, as well as just working with them and using them
  • There could still be issues I haven't found, so it could use more testing

… still need changing and/or adding.

Some notes
- A lot of cleanup needed, the classes implementing `ISession` and other interfaces are still missing several methods/properties
- Some changes to trading sessions will be needed, as they aren't stored in a single trading-only-sessions list
- `TickRateMultiplier` methods will need changing to actually use the reworked
- `ISession` and the other interfaces will have to be moved to the API
- The API should preferably expose abstract session classes to ensure any additions will be compatible in the future due to older implementations inheriting new members
- The location (and existence) of `IIdBlockProvider` is subject to change - it may get removed, renamed, moved, etc.
… session-rework

# Conflicts:
#	Source/Client/Comp/Game/MultiplayerGameComp.cs
#	Source/Client/Comp/Map/MultiplayerMapComp.cs
#	Source/Client/Comp/World/MultiplayerWorldComp.cs
- The session is only added if there's any pause lock added by a mod
- The session will only pause itself if any of the pause locks does so as well
- Once we update the API to include sessions, pause locks should be marked as obsolete
It was causing issues, and everything works fine without it.
Also error for semi persistent data sync now says writing semi persistent instead of exposing
The intention is to move it to MP API as a way to provide a way for other mods to interact with the session manager.

Currently needs a little bit more documentation (currently all documentation was moved from SessionManager).
@SokyranTheDragon SokyranTheDragon added the 1.4 Fixes or bugs relating to 1.4 (Not Biotech). label Nov 6, 2023
Removed `ISession`, `IExposableSession`, and `ISemiPersistentSession` in favor of abstract classes `Session`, `ExposableSession`, and `SemiPersistentSession`.

There was likely no class besides `Session` that would implement `ISession`, so it was removed. The other 2 were removed in favor of turning them into abstract subclasses of `Session`, which will allow for expanding them further in the future, and prevent people from implementing both interfaces (which was not supported).

All the xml docs were moved from interfaces to their respective classes.
@SokyranTheDragon
Copy link
Member Author

Pushed a commit that removed ISession, IExposableSession, and ISemiPersistentSession in favor of abstract classes Session, ExposableSession, and SemiPersistentSession. It was basically pointless to have those 3 interfaces. If needed it can be reverted.

…MP API

This is done in preparation of an update to MP API
- Added a single parameter (Map) constructor to all `Session` subclasses with docs to inform devs using the API of a mandatory constructor
- `ExposableSession` will now be provided the `Map` parameter by the `SessionManager`
- `SemiPersistentSessions` won't sync a map on a per-session basis, instead being provided the `SessionManager` map
It first checks if it's a subtype of `Session`, and will show an error if it isn't
@SokyranTheDragon
Copy link
Member Author

I've added a sync dict entry for ISessionWithTransferables. It originally worked as it inherited from ISession, however - due to dropping the 3 interfaces it ended up no longer being syncable. This should fix it.

@SokyranTheDragon
Copy link
Member Author

Working a bit with the new sessions, I've encountered a specific (and possibly rather niche) scenario.

I'm in a situations where I need to keep an event/quest/etc. map temporarily paused and alive. The mod itself would do it itself, but due to needing for syncing and some patches, I'm in a situation where it's not possible.

Keeping the map paused is rather easy with the sessions. However, this still requires the "keep alive" aspect implemented, so this made me wonder:

Would it be worthwhile to include to include IBlocksMapRemoval interface, or something along those lines (like ISessionBlockingMapRemoval). This would likely be done by patching Verse.MapPawns::AnyPawnBlockingMapRemoval property, where we'd likely checking the local (and possibly the global, depending on implementation) session managers for sessions blocking the map from being removed.

Anyway, this is just a minor thing to possibly consider. Checking this PR made me realize there are conflicts, so I'll look into resolving them in the next day or two.

… session-rework

# Conflicts:
#	Source/Client/Comp/World/MultiplayerWorldComp.cs
@SokyranTheDragon
Copy link
Member Author

Merged with the master branch and resolved the merge conflicts

Zetrith pushed a commit that referenced this pull request Dec 27, 2023
@Zetrith
Copy link
Member

Zetrith commented Dec 27, 2023

Merged

@Zetrith Zetrith closed this Dec 27, 2023
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