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

Submerging refactor #15894

Merged
merged 35 commits into from
Jun 15, 2024
Merged

Submerging refactor #15894

merged 35 commits into from
Jun 15, 2024

Conversation

Lumipharon
Copy link
Contributor

@Lumipharon Lumipharon commented May 18, 2024

About The Pull Request

Probably TM first in case something borks.

Refactors submersion code, making it generic instead of specific to liquid turfs.
Tall grass and fire now partially submerges mobs, and this can easily be extended to other stuff where appropriate.
image

Hunter in grass, human in grass, human in grass AND water, human just in water.

Items are partially obscured by liquid turfs (but not by contents like tallgrass, for gameplay reasons)
image

Vehicles also are also obscured now.
image

Also fixes #15865

Also while I was doing it made tallgrass also have a footstep override like catwalks etc.

Removed an init signal which was only used for one thing because Entered wasn't called and simply... called Entered. This will likely fix a whole bunch of extremely minor bugs when spawning things in. I can't see any issues this could cause, but let me know if there is something.

Why It's Good For The Game

Better code, more applicable to other stuff, look good.

Changelog

🆑
add: Tall grass and fire now partially obscured you similar to water
add: Items now sink in water, partially obscuring them
add: Vehicles canbe submerged like mobs
fix: fixed throws messing up pixel_y offset over bodies of water
refactor: refactored submersion code
/:cl:

@tgstation-server tgstation-server added Sprites Changes to .dmi file. Refactor Improves underlying code to make systems more modular and functional. Fix Fixes an issue with the game. Feature New interesting mechanics with new interesting bugs labels May 18, 2024
Copy link
Member

@TiviPlus TiviPlus left a comment

Choose a reason for hiding this comment

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

I dont think making a proc thats called on every move for every movable is a good idea?
why can't it be an element on the turf

@TiviPlus
Copy link
Member

looks really cool thou

@Lumipharon
Copy link
Contributor Author

I dont think making a proc thats called on every move for every movable is a good idea? why can't it be an element on the turf

What would be a practical way to make it work as an element, considering it would be required for normal turfs that have applicable atoms on them (i.e. floor with tallgrass), but that atom could move or be deleted.
i.e. if its just on a floor, then removing the element is fine, but if its on a water turf, we wouldn't want to remove the element. What if there was tall grass AND some other atom that required the element, that wasn't also destroyed?

I guess we could just recheck the height/depth turf procs and exclude qdeleted things before removing the element or something.

Let me mess around.

@Lumipharon
Copy link
Contributor Author

Ok I channeled some powers and created a element and component combo that I am actually happy with.

@TiviPlus TiviPlus added the Test Merge Candidate This PR has been reviewed and is ready for testing, unless something changes. label Jun 1, 2024
comfyorange added a commit that referenced this pull request Jun 1, 2024
comfyorange added a commit that referenced this pull request Jun 1, 2024
comfyorange added a commit that referenced this pull request Jun 3, 2024
comfyorange added a commit that referenced this pull request Jun 4, 2024
comfyorange added a commit that referenced this pull request Jun 4, 2024
comfyorange added a commit that referenced this pull request Jun 5, 2024
comfyorange added a commit that referenced this pull request Jun 5, 2024
comfyorange added a commit that referenced this pull request Jun 5, 2024
comfyorange added a commit that referenced this pull request Jun 5, 2024
comfyorange added a commit that referenced this pull request Jun 5, 2024
comfyorange added a commit that referenced this pull request Jun 5, 2024
comfyorange added a commit that referenced this pull request Jun 5, 2024
@Lumipharon
Copy link
Contributor Author

need to fix a big, untm'd.

@Lumipharon Lumipharon added Do Not Merge Pull request should not be merged due to design conflict or being a temporary change. and removed Test Merge Candidate This PR has been reviewed and is ready for testing, unless something changes. labels Jun 6, 2024
@Lumipharon Lumipharon removed the Do Not Merge Pull request should not be merged due to design conflict or being a temporary change. label Jun 6, 2024
@Lumipharon
Copy link
Contributor Author

ok this should be good to merge now

@Lumipharon Lumipharon requested a review from TiviPlus June 6, 2024 01:18
@github-actions github-actions bot added the Merge Conflict Pull request is in a conflicted state with base branch. label Jun 6, 2024
@tgstation-server tgstation-server removed the Merge Conflict Pull request is in a conflicted state with base branch. label Jun 6, 2024
. = ..()
if(!isturf(target))
return ELEMENT_INCOMPATIBLE
//override true as we don't look up if the turf already has the element
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the right way to handle things WRT trying to add an already present element? I'm not super familiar with them.

@@ -132,6 +133,8 @@
/// inherited from riding vehicles
#define VEHICLE_TRAIT "vehicle"

#define TRAIT_SUBMERGED "trait_submerged"
Copy link
Contributor

Choose a reason for hiding this comment

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

Having TRAIT_SUBMERGED and SUBMERGED_TRAIT seems kinda

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its kinda wew, but its following the naming convention so eh.

code/game/atoms/_atom.dm Show resolved Hide resolved
var/atom/movable/owner = parent
RegisterSignal(owner, COMSIG_MOVABLE_MOVED, PROC_REF(move_submerge_element))
for(var/atom/movable/AM AS in owner.loc) //we remove the submerge and reapply after owner is connect
AM.set_submerge_level(null, owner.loc, duration = 0.1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it important to clear them first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otherwise it will permanently screw up offsets for anything submerged on the tile. Pretty much a lot of this code is to avoid an atom level var, really.

///List of all filter removal timers. Glob to avoid an AM level var
GLOBAL_LIST_EMPTY(submerge_filter_timer_list)

///Sets the submerged level of an AM based on its turf
Copy link
Contributor

Choose a reason for hiding this comment

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

Submerge level isn't an actual thing AMs have, right? It looks like this is just setting up a filter/animate.

code/game/turfs/liquid_turfs.dm Show resolved Hide resolved
code/game/turfs/liquid_turfs.dm Show resolved Hide resolved
code/game/turfs/liquid_turfs.dm Show resolved Hide resolved
@@ -12,3 +12,6 @@
remove_movespeed_modifier(MOVESPEED_ID_BULKY_DRAGGING)
return
add_movespeed_modifier(MOVESPEED_ID_BULKY_DRAGGING, TRUE, 0, NONE, TRUE, .)

/mob/living/set_submerge_level(turf/new_loc, turf/old_loc, submerge_icon = 'icons/turf/alpha_64.dmi', submerge_icon_state = "liquid_alpha", duration = cached_multiplicative_slowdown + next_move_slowdown)
Copy link
Contributor

Choose a reason for hiding this comment

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

Having what's basically assignment logic in the proc declaration is weird but I guess it works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I already have the check in the base proc, so didn't see any value in requiring more copy paste checks for every version of the proc.

@QualityVan QualityVan merged commit 74950ce into tgstation:master Jun 15, 2024
35 checks passed
github-actions bot added a commit that referenced this pull request Jun 15, 2024
@Lumipharon Lumipharon deleted the liquid_mod branch June 15, 2024 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New interesting mechanics with new interesting bugs Fix Fixes an issue with the game. Refactor Improves underlying code to make systems more modular and functional. Sprites Changes to .dmi file.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pounce through water offset
4 participants