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

make xHighLevelStarTrekDoor.py to be less bad #1470

Merged
merged 5 commits into from
Oct 3, 2024

Conversation

Hazado
Copy link
Contributor

@Hazado Hazado commented Aug 16, 2023

Not saying this is better but its less bad

Removes age owner from controlling the triggering of the doors
Removes the SendNote() stuff it was doing
Removes the doorstack tracking
Makes the door opening and closing rely on the SDL triggering
Makes the SDL trigger only happen by the person who set off the door sensor

I think ive fixed some of the race conditions but this is a multiplayer script and those are hard to troubleshoot

@Hazado
Copy link
Contributor Author

Hazado commented Aug 16, 2023

I did have one bug happen during my local testing (two clients on the same computer connecting to my local dockersand)
If the avatar that triggered the door sensor quit without leaving the sensor region, the sensor wouldn't trigger anymore
Think its a multi-client on the same computer bug and not one with the script

Copy link
Member

@Hoikas Hoikas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a quick review, but I did notice that there is a duplicated variable -- self.SDL["DoorState"] and self.DoorState. It would be nice if only one of these were used.

Scripts/Python/xHighLevelStarTrekDoor.py Outdated Show resolved Hide resolved
Scripts/Python/xHighLevelStarTrekDoor.py Outdated Show resolved Hide resolved
@Hazado
Copy link
Contributor Author

Hazado commented Aug 17, 2023

This is a quick review, but I did notice that there is a duplicated variable -- self.SDL["DoorState"] and self.DoorState. It would be nice if only one of these were used.

Yeah, one is a script SDL and the other a class variable. I should probably just use the script SDL

@Hazado
Copy link
Contributor Author

Hazado commented Aug 17, 2023

Added the requested changes

@Hazado
Copy link
Contributor Author

Hazado commented Aug 18, 2023

Tested this change on minkata alpha server.
Doors always worked with one bug
If the person who opened the doors links out or quits while in the sensor region, the doors will stay open until someone else enters the sensor region.

@Hoikas
Copy link
Member

Hoikas commented Aug 19, 2023

Tested this change on minkata alpha server.

Is "minkata alpha server" now on H'uru? 😮

@Hazado
Copy link
Contributor Author

Hazado commented Aug 19, 2023

Tested this change on minkata alpha server.

Is "minkata alpha server" now on H'uru? 😮

Im ready to test this on trolland :)

Copy link
Member

@Hoikas Hoikas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we've now changed the SDL tracking and door animations to be done locally, we no longer need to acquire the exclusive server lock when someone enters the door region sensor. Each player is responsible for doing their own book-keeping, so there is no message storm or race condition concerns. I haven't looked at the PRPs yet, so I don't know if these are plVolumeSensorConditionalObject (requests exclusive server lock) or plVolumeSensorConditionalObjectNoArbitration (no server lock) in your fixed PRPs. We can fix the issue by, in the OnServerInitComplete() or OnInit() routines doing

rgnSensor.volumeSensorNoArbitration(True)

to force the issue. This should improve the reliability when many players are crossing the region boundary.

Further, since we are making the script "less bad", I think we should convert the doorSDLstates dict to something more Pythonic, like an IntEnum.

Scripts/Python/xHighLevelStarTrekDoor.py Outdated Show resolved Hide resolved
Scripts/Python/xHighLevelStarTrekDoor.py Outdated Show resolved Hide resolved
Comment on lines 184 to 207
def UpdateDoorState (self, StateNum):
if StateNum != self.DoorState:
if StateNum != self.SDL['DoorState'][0]:
ageSDL = PtGetAgeSDL()
self.DoorState = StateNum
self.SDL['DoorState'] = (StateNum,)
self.SendNote('DoorState='+str(StateNum))
PtClearTimerCallbacks(self.key)
PtAtTimeCallback(self.key, 30, 1)
if self.DoorEnabled == 0:
return

if self.DoorState == doorSDLstates['opening']:
self.SendNote("respOpenDoor;0")
PtDebugPrint("xHighLevelStarTrekDoor: Notifying Clients to play Open Door Responder")

elif self.DoorState == doorSDLstates['closing']:
self.SendNote("respCloseDoor;0")
PtDebugPrint("xHighLevelStarTrekDoor: Notifying Clients to play Close Door Responder")

elif self.DoorState == doorSDLstates['open']:
PtDebugPrint("xHighLevelStarTrekDoor: Updating Age SDL to Open")
return
if self.SDL['DoorState'][0] == doorSDLstates['opening']:
ageSDL[strDoorClosedVar.value] = (0,)

elif self.DoorState == doorSDLstates['closed']:
PtDebugPrint("xHighLevelStarTrekDoor: Updating Age SDL to Closed")
elif self.SDL['DoorState'][0] == doorSDLstates['closing']:
ageSDL[strDoorClosedVar.value] = (1,)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be improved greatly by making the door state into a property.

Scripts/Python/xHighLevelStarTrekDoor.py Outdated Show resolved Hide resolved
playerID = PtGetLocalPlayer().getPlayerID()
triggerstr = "rgnTriggerEnter%d" % playerID
self.SendNote(triggerstr)
if event[0] == 1 and event[1] == 1:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if there is a less tedious way to do this?

elif self.DoorState == doorSDLstates['open']:
respOpenDoor.run(self.key,fastforward=1)

if self.SDL['DoorState'][0] in (doorSDLstates['opening'], doorSDLstates['movingopen'], doorSDLstates['opentoclose']):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if self.SDL['DoorState'][0] in (doorSDLstates['opening'], doorSDLstates['movingopen'], doorSDLstates['opentoclose']):
if self.SDL['DoorState'][0] in (doorSDLstates.opening, doorSDLstates.movingopen, doorSDLstates.opentoclose):

doorSDLstates should now be using members thusly. You'll need to apply these changes throughout the file.

Scripts/Python/xHighLevelStarTrekDoor.py Outdated Show resolved Hide resolved
Comment on lines +63 to +71
class doorSDLstates(IntEnum):
closed = 0
opening = 1
open = 2
closing = 3
opentoclose = 4
closetoopen = 5
movingopen = 6
movingclosed = 7
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I want us to work on these state names so that what is going on is a little more clear. I know that I still need to open this file in VS Code (instead of viewing the diff, which is basically useless) to really understand what all is going on here.

@Hazado Hazado marked this pull request as ready for review November 10, 2023 18:15
Hazado and others added 5 commits December 1, 2023 08:39
removes sendnote()
Co-authored-by: Adam Johnson <AdamJohnso@gmail.com>
Co-authored-by: Adam Johnson <AdamJohnso@gmail.com>
Co-authored-by: Adam Johnson <AdamJohnso@gmail.com>
@Hazado Hazado force-pushed the LessComplicatedStarTrekDoors branch from 28dacb6 to 9f872d0 Compare December 1, 2023 16:39
Copy link
Member

@Hoikas Hoikas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's still room for improvement here, but I think we should go ahead and bring this in.

@Hoikas Hoikas merged commit f5af9cb into H-uru:master Oct 3, 2024
18 checks passed
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.

2 participants