Repository navigation
Conversation
9cf7866 to
bbde0e3
Compare
rfortier
changed the title
e4475b6 to
25fa8c9
Compare
|
Substantial logic change. Remote updates received while playing a scene are ignored. Unless they include a SceneEnd flag. Those updates are sent when a scene ends and forces everyone to accept the update to catch up. Many variations of scene support have been tried, this seems to be the best compromise. Unless multiple players click the option to continue. That's often going to do something unpredictable, don't do that. To unstick players, at Scene End, send a "poke" quest stage update that is not deduped, to catch everyone up. A future improvement may send such a poke every time player interaction causes a quest stage change, but have to figure out how to detect that. |
5e2f603 to
3323bb6
Compare
48729e5 to
4804437
Compare
877d649 to
3d2eb3c
Compare
|
Did a submission cleanup pass. Deleted all the dead (#if 0) code that didn't work or didn't help. Kept as a separate commit so you can see that code and the notes on why it didn't work if anyone works on this in the future. Turned down all the logging to a more modest amount. There are a couple of [critical] logs that aren't exactly errors, they are still experimental code paths that haven't been fully tested because they have not yet been seen in the wild. I might know where one is hiding, though. It's ready for review. I've gotten the critique already that it is "too big," but most of that is logging. It's a repeating pattern of setting up a bunch of state / status variables so they can be used in log messages, and then a lot of logs, what it took to figure out what was happening or broken, and then proving the fix in a rapid-fire distributed state machine. Could delete the logging now that it works, but I don't recommend it. I think there are still more edge cases we will find, and we'll need the same logs to fix the AI Package support, which will fix several more cases where the party Leader must be the one to advance the quest. The heart of the change is in the two QuestService.cpp files. Most of the rest is RE:: support and an easy read. |
…ssion (PR tiltedphoques#769) # Conflicts: # Code/client/Games/Skyrim/Forms/TESQuest.cpp # Code/client/Games/Skyrim/Forms/TESQuest.h # Code/client/Services/Generic/MagicService.cpp # Code/client/Services/Generic/QuestService.cpp # Code/server/Services/QuestService.cpp
… quest progression PR tiltedphoques#769, includes tiltedphoques#854) # Conflicts: # Code/client/Games/Misc/SubtitleManager.cpp # Code/client/Games/Skyrim/Actor.cpp # Code/client/Games/Skyrim/Forms/TESQuest.cpp # Code/client/Games/Skyrim/Forms/TESQuest.h # Code/client/Services/Generic/CharacterService.cpp # Code/client/Services/Generic/QuestService.cpp # Code/encoding/Messages/ClientMessageFactory.h # Code/encoding/Messages/ServerMessageFactory.h # Code/server/Services/QuestService.cpp
The tiltedphoques#848 merge kept dev's message lists and added RequestQuestSceneUpdate and NotifyQuestSceneUpdate to them, but not their includes.
Use StopQuest's return value instead of overwriting it with true after the if/else, and set bResult = true in the already-stopped branch. StopQuest only returns false when its GetById lookup fails, which OnQuestUpdate already checks above, so this changes no behaviour. It matches tiltedphoques#848's approach (wasUpdated = StopQuest(formId)).
…#848) If the SceneMaster leaves, is kicked or disconnects (all go through RemovePlayerFromParty), its id stayed in the party's QuestStageDedupHistory: remaining members' SceneEnd never fanned out / forced the stage, and a re-Begin of the same scene kept the stale master. Reset it so the next Begin re-elects. Leader change does not touch SceneMaster (initiator, not leader). Server PartyService only; depends on tiltedphoques#848 (83d0b0be). Original lane: STR Party (local/str-party-exp @ c2f8dd85), staged by Build after static review.
Cached-update resync (can be older than server 30s dedup) re-emitted every applied stage/start/stop to the leader since tiltedphoques#848 dropped the OnEvent override checks. Record expected echoes per formId+stage (5s bound, sync or async), drop in OnEvent. Original lane: STR Quest (local/str-exp-quest @ b637627d), staged by Build after static review.
…only Party Leader), but it introduced a bunch of problems with duplicate quest triggers. This rework deals with it (hopefully). The most fundamental problem is that ScopedQuestOverride() doesn't work, because quest progress is executed on a different thread; the ScopedQuestOverride() is destructed before the quest OnEvent...() routines are invoked. So all the Members send their updates to the server even when told not to. The server just ignored them all before, but now with Member quest progression enabled, it ends up causing replicated quest updates. Some quests ignore it, many break. The engine is in control of the content of events passed to the OnEvent routines. I could not find a reliable way to pass any context to the OnEvent routines through that barrier (it runs async, so matching up was nigh-on impossible). So ended up implementing a dedup history server-side, where there is enough context to figure this out. The worst-case flow (when a Member triggers progress), is: 1) Member sends update. 2) Update is forwarded to Leader, only. 3) Leader OnEvent hooks send the Leader update to the server. 4) Leader SendsToParty 5) Party members reflect the updates to the Server, but are dropped by the dedup history because the Leader has already sent it. 6) Entries in the dedup history time out in seconds (needs some tuning), so quests can rewind and stages can be triggered multiple times to evaluate. Partial support for Remote actors to run AI packages when in-Scene
Has some value in a commit to say "already tried that."
Members' scripts often reach the same quest stage on their own, and a stage relayed mid-scene can push another client's copy of the scene past its own scripts (MQ01's vision ended before it began). Replace the dedup history and SceneMaster rework with per-party stage tracking on the server, skipping stages a client already reached, and having the leader hold member stages until the quest's scenes end
…ix/Rework-PR-769-party-member-quest-progression
112493d to
4dece28
Compare
The leader held member quest stages until all of the quest's scenes ended, but many scene phases wait for a stage before they can end. A member picking a dialogue option that sets such a stage left the leader's scene stuck in that phase for good. Now the leader checks every frame and releases a held stage once the current phase's dialogue is done and its completion conditions would pass with it. At a scene end, held stages apply in order up to one that another scene still waits on. Held stages are dropped once reached locally, when the quest stops, and on party leave or disconnect. Adds BGSScenePhase, TESConditionItem and BGSScene::currentPhaseIndex, verified against the 1.6.1170 binary.
While a scene plays, the leader holds stages a member reaches. If the member's quest stopped before they were released, the leader dropped them as soon as the quest stopped there, so their fragments never ran for the leader. A member's stages always arrive before its stop, and the leader applied them in that order before the hold, so apply any held stages before stopping the quest.
No behavior change. The held-stage lookup now hands callers the stage list and drops empty entries itself, and the two catch-up paths in OnQuestUpdate are now one
|
Changelog entry: "Reworked quest progression in a party, greatly reducing broken quests and stuck cutscenes" |
miredirex
changed the title
2 participants
The community loves Members being able to advance quests (instead of only Party Leader), but it introduced a bunch of problems with duplicate quest triggers. This rework deals with it (hopefully).
A large credit goes to co-author @miredirex for reverse engineering a couple more methods to make this work, especially having more awareness of when a Player is in a scene (BGSScene) or not. That's one key to making cutscenes sync, or at least sync much, much better. That, and deduplicating the storm of duplicate attempts to advance the quest stage that was introduced with Member-can-progress-quests PR #769. That introduced several bugs that are fixed here.
A lot of the effort in this PR was figuring out the real-world situations in which quests can and cannot rewind, and making sure the fix supported that. You can find the information in ck.uesp.net starting with SetStage(), but it isn't all consolidated in one place. Find comments in the code, it's too long and complicated to re-explain here (likely introducing errors)
The most fundamental problem found in the old code is that ScopedQuestOverride() doesn't work, because in many cases quest progress is executed on a different thread; the ScopedQuestOverride() is destructed before the quest OnEvent...() routines are invoked. So all the Members send their updates to the server even when told not to. The server just ignored them all before, but now with Member quest progression enabled, it ends up causing replicated quest updates. Some quests ignore it, many break.
The engine is in control of the content of events passed to the OnEvent routines. I could not find a reliable way to pass any context to the OnEvent routines through that barrier (it runs async, so matching up was nigh-on impossible). So ended up implementing a dedup history server-side, where there is enough context to figure this out.
The worst-case flow (when a Member triggers progress), is:
There's also logic to keep track of the SceneMaster, the player that initiated the scene. This has proven to be less useful than hope, it was to be used to sync everyone's Scene state to the SceneMaster. But the scene start/stop routines have so many side-effects, the logic made things worse more often than better. So now, state is tracked in case we find more use for it, but largely doesn't do anything.
The one exception: players can get stuck in dialogues that have been answered by the SceneMaster. Most times, they are fixed by a quest stage transition, but I have seen scenes that don't end with a quest stage change. So Scene End triggers a forced quest stage change. It's ignored by the client if they've already done it, so this one helps more than it hurts.
Partial support for Remote actors to run AI packages when in-SceneThat comment in the commit is obsolete, the support was yanked as too incomplete.