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

Publicize CurrentRestMode #1965

Merged
merged 1 commit into from Nov 20, 2020
Merged

Conversation

jefetienne
Copy link
Contributor

This is for issue #1962.

This PR essentially would replace the utility of PlayerEntity.IsLoitering and PlayerEntity.IsResting, but may be kept for mod compatibility reasons. I also have no problem with removing them if requested. :)

I publicized the RestModes enum and kept it in DaggerfallRestWindow, but I can move it elsewhere to DaggerfallUnityEnums if mod compatibility is also not an issue.

@Interkarma
Copy link
Owner

I'm happy to leave in IsLoitering and IsResting for overall compatibility. Thank you for fix! :)

@Interkarma Interkarma merged commit fdb24e8 into Interkarma:master Nov 20, 2020
@petchema
Copy link
Collaborator

petchema commented Dec 14, 2020

I'm currently tracking some regression:
When trying to sleep (say at night, to increase the likeliness of an encounter) and get interrupted, getting out of enemy reach and trying to rest again, instead of having to choose a resting option, you immediately get a "You wake up." message on top of a countdown at 0
you wake up
A second attempt works fine.

And git bisect points at commit 29e2616

@petchema
Copy link
Collaborator

petchema commented Dec 14, 2020

Test game save, if you need one
SAVE871.zip

(thinking of it, it may not be the best save, it's in the middle of the ocean thanks to an issue with Interesting Terrains!)

@Interkarma
Copy link
Owner

Maybe the removal of protected RestModes currentRestMode = RestModes.Selection;? This default no longer appears to be set anywhere.

If not, should we create a quick issue for this one? I'm starting to unplug a little for Christmas and don't want to lose track.

@jefetienne
Copy link
Contributor Author

I think the issue is because when the window is pushed, it may still be using the last rest mode stored in GameManager because of an interruption. I'll take a look at it.

@petchema
Copy link
Collaborator

That was my guess too, but I'm not sure how to emulate that back with a property.
I did

diff --git a/Assets/Scripts/Game/UserInterfaceWindows/DaggerfallRestWindow.cs b/Assets/Scripts/Game/UserInterfaceWindows/DaggerfallRestWindow.cs
index c359ac45c..273777315 100644
--- a/Assets/Scripts/Game/UserInterfaceWindows/DaggerfallRestWindow.cs
+++ b/Assets/Scripts/Game/UserInterfaceWindows/DaggerfallRestWindow.cs
@@ -116,6 +116,7 @@ namespace DaggerfallWorkshop.Game.UserInterfaceWindows
             : base(uiManager)
         {
             this.ignoreAllocatedBed = ignoreAllocatedBed;
+            currentRestMode = RestModes.Selection;
         }
 
         #endregion

and it seems to work...

@jefetienne
Copy link
Contributor Author

jefetienne commented Dec 15, 2020

That was my guess too, but I'm not sure how to emulate that back with a property.
I did

diff --git a/Assets/Scripts/Game/UserInterfaceWindows/DaggerfallRestWindow.cs b/Assets/Scripts/Game/UserInterfaceWindows/DaggerfallRestWindow.cs
index c359ac45c..273777315 100644
--- a/Assets/Scripts/Game/UserInterfaceWindows/DaggerfallRestWindow.cs
+++ b/Assets/Scripts/Game/UserInterfaceWindows/DaggerfallRestWindow.cs
@@ -116,6 +116,7 @@ namespace DaggerfallWorkshop.Game.UserInterfaceWindows
             : base(uiManager)
         {
             this.ignoreAllocatedBed = ignoreAllocatedBed;
+            currentRestMode = RestModes.Selection;
         }
 
         #endregion

and it seems to work...

I think maybe putting it in OnPush() might be better? I forget how often the methods Start(), Setup() and the Constructor() are called when opening the window multiple times, but I know OnPush() is every time.

@petchema
Copy link
Collaborator

petchema commented Dec 15, 2020

Well, I think removed line

protected RestModes currentRestMode = RestModes.Selection;

was only executed once too, when the window was built...
I'm not sure if setting it explicitly in the constructor is "correct", but I think it should be equivalent to what was there before

petchema pushed a commit to petchema/daggerfall-unity that referenced this pull request Dec 15, 2020
Description of the bug in comments of the Interkarma#1965 PR
@petchema
Copy link
Collaborator

If not, should we create a quick issue for this one? I'm starting to unplug a little for Christmas and don't want to lose track.

I created a PR

@Interkarma
Copy link
Owner

Great work, thank you @petchema and @jefetienne.

Interkarma added a commit that referenced this pull request Jan 22, 2021
@jefetienne jefetienne deleted the restMode branch February 1, 2022 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants