-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Remove ledge trap hack #78254
base: master
Are you sure you want to change the base?
Remove ledge trap hack #78254
Conversation
74821dd
to
31e774d
Compare
It would indeed be good to eventually replace the current support logic with something more realistic, but that's a huge undertaking, and the first step would be to devise a model that won't gum up the game with molasses when large scale destruction occurs (e.g. house on fire or blown up by explosives). Falling through and destroying "terrain" is really part of the support functionality, as things that's too weak to support a creature also would provide negligible support to adjacent "terrain". It could even be that separation of the current terrain layer into an actual terrain layer and a construction/biology layer (buildings, trees, etc.) could help with this: Nothing on the construction/biology layer means no support calculations for moving critters, as well as no need to extend support calculation ranges (unless you want to deal with soil roofs). |
Ye I misused ideally there, obviously a proper support system that takes into account other tiles and actual physics would be the ideal I just don't know how viable making that happen is given we don't even prevent outright 0 neighbour floating terrain rn. |
The levitating terrain is a result of the randomness of the current collapse logic together with it checking only immediate neighbors. This means the logic can decide to leave one tile first and then remove its last neighbor after that. In addition to that, the logic also makes levitation more probable if the patch is large, as each levitating tile "support" the others. |
64bcbd0
to
4901acc
Compare
@@ -1719,11 +1720,11 @@ | |||
"name": "open air", | |||
"description": "An area of open air.", | |||
"//": "Actually, this is a thin membrane of bullshit to keep wasp eggs levitating.", | |||
"//1": "Why do we want eggs to levitate??", |
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 this is because they're actually supported from above.
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 is something the current support logic doesn't support (nor are overhangs, which doesn't stop JSON specified buildings to have them), so some form of hand waving is needed.
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.
Auto-requesting reviews from non-collaborators: @Night-Pryanik
Not the most ideal looking spot for this but its the only place all the calls I could find filter down to
cf8e70d
to
c27b944
Compare
4e8a51a
to
14b80cf
Compare
e9cb0f7
to
a9dd7e7
Compare
The random vehicle test fail really hates this one .-. |
That damnable test doesn't like me either, and refuses to fail in a repeatable fashion. |
035bf29
to
38e4433
Compare
…nd forgetting about them is not great?
38e4433
to
ab8142d
Compare
Summary
Infrastructure "Falling is no longer handled by traps"
Purpose of change
This hack was gross and didn't look that bad to remove
Describe the solution
Currently I've replaced the tr_ledge checks with NO_FLOOR checks
Actually falling uses the same logic as before except it's now a function of map instead of a trapfunc
Add new type trap migration and migrate tr_ledge to tr_null
Also migrates t_hole to t_open_air
Describe alternatives you've considered
This needs more nuance than just NO_FLOOR, but it should be ok for this PR, I've checked all the in repo usages and they make enough sense. I imagine regardless of what I do there's going to be at least a couple of obscure bugs this causes so I'm not aiming to be too ambitious here I just want as few bugs as possible
Ideally we'd have maximum weights floors could support, for example skylights should be able to support a rat and a pocket watch but if a woodland wight or an itemised fridge is dropped on it it should break. Furniture also needs weight assigning to it in some form whether automatically or manually.
Given that this has 0 test fails without tweaking any I presume we have a distinct lack of tests covering gravity related stuff
A debug no clip trait that lets you ignore gravity, < and > to different z levels any time and walk through walls could be convenient for some stuff (or a lesser ignores gravity but can't go through solid matter)
It's pretty weird that we have floor caches that aren't checked against in process_falling but changing that would be a significant PR in itself bc it would need dirtying much more from a cursory glance
Testing
Player, monster, vehicle falling on move works
"Zed walking" still "works"
Climbing up/down works
Hulk smash off a roof works
Grapple attacks off a roof pull you off and result in fall damage, results in zed walking if grappler tries to pull you diagonally down onto them
Monsters and NPCs that spawn/are loaded in the air fall down immediately
Gliding works
Martial techniques with knockback used to knock things off rooves work
Throwing/dropping/AIMing items off rooves works
Spawned vehicles fall after a turn where appropriate which seems fine
Chopping down a tree through use action or bashing results in a monster/Character on top to fall immediately regardless of whether it uses all your moves
Dragging furniture off an edge causes it to fall immediately but gives an error (which it already does in live, see #62171 (comment), not sure what the ideal fix is here, the error should only fire for initial mapgen placements not stuff that happens post initial map load)
Additional context
I am mildly concerned about the performance implications of testing a terrain flag any time anything moves but hopefully it's negligible alongside all the other logic
Also concerned that this passed all the tests without adjusting any of them