-
Notifications
You must be signed in to change notification settings - Fork 360
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
Changes supporting falling actions #1848
Conversation
Iirc patients can't fall inside rooms. So think you should be fine unless females are stripping around the hospital |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe out of scope a little but falling's line 28 should be a colon and not a full stop (I think)
i.e. this line needs correcting
self.setMustHappen(true)
Forgot to copy that one across. The Stripped Female Patients won't get the updated on_ground animation in the afterload, so once they become normal ones, they will be restored with the old one, also if they transition from Standard to Stripped then back to Standard. Obviously not a problem on new saves. |
@mugmuggy May need to be it's own issue but it looks like some recent update means triggering falls in earthquakes is causing the adviser to repeatedly saying the "These people are ill" "How would you like it" etc. when the falling is on. |
I think we lost the GitHub logs on this one, if you wanted to try and regenerate them. |
Looks good to me, except for the problem spotted by Lewri. My suggestion is just further improvement of the code while you are at it. |
Unfortunately I don't think this one is true. I'm still see this even with your changes. |
|
Hmm. I've attached a webm here showing what's happening still. screen-recording.3.mp4 |
89cae9e
to
8ed9c0b
Compare
2e5c6fe
to
efa9195
Compare
else | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Invert the condition so the return can be done immediately after deciding it's not happening.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if not self.falling_anim or not self:canPeeOrPuke(current) or not self.has_fallen == 1 then return end
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the function ends there as well, just delete the "else return" instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of your long condition, you can break it into 3 parts, and bail out as soon as one fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An unsure why it needs a "pee or puke" function. Probably it was re-used without adapting the name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's 3 options listed now. Which one is the preferred one here? 😅
An unsure why it needs a "pee or puke" function. Probably it was re-used without adapting the name.
Very old code :) df84d21
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which one is the preferred one here?
Hmm, you're asking such difficult questions :)
I'd probably go for deleting the else return
, as it's not doing anything useful.
Very old code :)
Point is that it's being used for something else than pee or puke. The name of the function doesn't cover all it's purposes anymore. You can extend it with the new purpose, but maybe a more general name is better.? I pondered about a name and came up with atFreeTile
or atOpenTile
. Not sure these are good names though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function checks more than tiles, it has 3 steps:
- Is the current action interruptible?
- Does the patient have going home status?
- Finally check if the current tile is buildable, owned, and in a hospital
It's likely the extend name option is going to be the easiest here; can't think of a good general name either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with extending the name.
Good point, so perhaps this is not just 1 function, it's 2 or 3 functions mixed into one?
The first 2 steps seem related to the question "is it possible and does it make sense to insert a new action in the queue?", while the 3rd step seems about an empty tile (which probably should be abstracted into an isAtEmptyTile
function, but that is out of scope here).
I am not sure about the "inHospital" check for the current usage of this function. For pee and puke it makes sense, since a handyman needs to cleanup the mess, but falling doesn't need to be cleaned. Not a serious problem though, falling patients at the middle of some road are never seen anyway.
If you want to fix things in that function, doing that in a separate patch is likely the better idea, imho.
Not sure why it needs a new section in the config file. Can't we have another setting in the "debug" settings section? May somewhat depend on the public though. |
As an example, for soundfont override on fluidsynth builds we don't give any indicators the setting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how good it is exactly, but let's find out :)
Everything should be cleared up now and ready. I also made this a debug menu item for easier toggling, Edit: appveyor can be ignored it can't get the lua source for some reason right now but it is accessible. |
LGTM too |
- Allow players to push people over when right clicking on them - Only announce a fall if the player pushed them over - Change falling to a debug setting, defaulted to false. - Fixes falling:28 - Renames canPeeOrPuke to canPeePukeOrFall - Adds falling as a debug menu item
for _, patient in ipairs(hospital.patients) do | ||
if not patient.in_room and patient.falling_anim then | ||
patient:falling(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be a probability roll here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, "tick" function eh? Should be a very low probability if you want to notice any of a delay, I guess.
Quick falling is perhaps more useful for testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it's every quake tick (which different from a regular tick), which in essence is a damage cycle completed.
Potentially its fine for now. If a patient can't fall at one part of the quake they may move and be able to later on. I think if it becomes too uniform then we can adjust later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree :) First make it work at all before starting to fine-tune it :)
Fixes #476
Describe what the proposed change does
I didn't activate the falling code in world, so these don't actually initiate anything but can be used for testing.
Doesn't as pointed out in the discussion #1846 attempt to address the issue of interrupted actions being incorrectly requeued for example, walking to room that is destroyed at the same time the fall action starts, which would requeue the interrupted walk to that room.
I had put a has_interrupted flag on the action and detected it as not to requeue, but not all interruptions are driven through setNextAction (interrupt_head in queue for example) and I think the other issue was actions like the walk which starts and does something, sets the interrupt handler, which means the action starts, rather than a todo_interrupt being detected to basically skip the action in the action start (seek_toilets_action_start for example and this is same issue with teleporting the drinks machine or jumping to/from bench when queue actions are interrupted as use_object also must start)