Skip to content

Add prop-break related functions #3300

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

deltamolfar
Copy link
Contributor

No description provided.

@wrefgtzweve
Copy link
Member

wrefgtzweve commented Apr 9, 2025

this seems pretty abuseable as it basically allows the owner to give full godmode to all their entities while they werent able to do this before

this will need a way to disable as this is no doubt going to be abused

the hooks starting with ~~~ dont make much sense, why name them like that?

also returning true in entitytakedamage would be more logical
image

@deltamolfar
Copy link
Contributor Author

Cvar and check if the ent is not npc should be made, will do.

Regarding tildas - they're there to bring down these hooks in stack order, although I'm not sure if they're even alphabetically ordered.

Regadring abuse - perhaps, but so far - couldn't make it abusable. Addons like contraption core work just fine. Maybe I should also restrit it to physical prop, so sents wouldn't be able to be mare invulnerable.

Regarding returning true - again, I don't want to break other addons like contraption core, as, in theory, returning true will result in other hooks in the stack not running, thus removing ability for other addons to do whatever they want

@thegrb93
Copy link
Contributor

thegrb93 commented Apr 9, 2025

also returning true in entitytakedamage would be more logical

Return true is bad because it prevents other damage hooks from firing. We want to remove the damage, not block the event.

Starfall's version of this function is limited to prop_physics class entities. Might be good to limit that here too.

@thegrb93
Copy link
Contributor

thegrb93 commented Apr 9, 2025

I'm not sure if they're even alphabetically ordered.

They aren't. It's random based on however lua iterates a string key'd table with 'pairs'.

@Astralcircle
Copy link
Contributor

Astralcircle commented Apr 9, 2025

I think we should store the unbreakable status of an entity in its own table rather than in a separate one.

Co-authored-by: Astralcircle <142503363+Astralcircle@users.noreply.github.com>
@wrefgtzweve
Copy link
Member

also returning true in entitytakedamage would be more logical

Return true is bad because it prevents other damage hooks from firing. We want to remove the damage, not block the event.

Starfall's version of this function is limited to prop_physics class entities. Might be good to limit that here too.

you shouldnt be running damage hooks when you're doing 0 damage

@thegrb93
Copy link
Contributor

Some things use the damage hook to tell if a prop gets hit tho, so they'd not detect hits on unbreakable things if you did that. It's kind of up to user interpretation though I guess.

@wrefgtzweve
Copy link
Member

https://gmodwiki.com/GM:PostEntityTakeDamage exists for those cases

@deltamolfar
Copy link
Contributor Author

https://gmodwiki.com/GM:PostEntityTakeDamage exists for those cases

Even tho it does, it does not mean that other addons use it. We don't want to break other addons, and there is no real benefit to returning true for us as well, only potential breakage of third-party addons.

@thegrb93
Copy link
Contributor

Try return true and see if the PostEntityTakeDamage works. If so then we should go with that.

@wrefgtzweve
Copy link
Member

other addons receiving 0 damage is going to cause issues
they expect actual damage in a damage hook
image someone dividing by the damage, now they'll be dividing by zero which basegame would never have done

@Astralcircle
Copy link
Contributor

Astralcircle commented Apr 14, 2025

Yes, why change the damage if returning true is supposed to block the damage?

@deltamolfar
Copy link
Contributor Author

Yep, just tested. Returning true prevents PostEntityTakeDamage.

Peek.2025-04-14.23-19.mp4

@thegrb93
Copy link
Contributor

Starfall uses the built in engine damage filter, which might do the same thing. Maybe doesn't matter? Is there a reason to detect damage on unbreakable props?

@deltamolfar
Copy link
Contributor Author

Well the non-breakable-by-default props still get both hooks correctly. I
should check out how sf does it, and if it's the same, then it is what it
is ig.

The return true tho is even worse, because some things may want just the
fact of damage, and not the damage amount itself, thus scaling dmg to 0 is
a better solution imho

@wrefgtzweve
Copy link
Member

If damage isn't being done it shouldn't be running damage hooks, this is unexpected behavior and will cause issues with other addons.

@wrefgtzweve
Copy link
Member

Could maybe be useful for there to be a hook to decide if a entity can be made unbreakable but considering it's only prop_physics i suppose it's fine as is

if not ValidAction(self, this, "break") then return end
if this:GetClass() ~= "prop_physics" then return self:throw("This entity can not be made unbreakable!", nil) end

this.wire_unbreakable = this:Health() > 0 and breakable == 0 and 1 or 0
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be this:Health() > 0 and breakable == 0 and true or nil

@Astralcircle
Copy link
Contributor

Astralcircle commented Apr 18, 2025

I already do this in my modified version of unbreakable tool and it causes absolutely no problems, because why should there be any problems? Why call another hook just to emphasize the lack of damage?

@wrefgtzweve
Copy link
Member

Once #3300 (comment) and swapping out ScaleDamage(0) to returning true are addressed this pr is good to go

@@ -714,6 +715,31 @@ e2function void entity:propBreak()
this:Fire("break",1,0)
end

hook.Add("EntityTakeDamage", "WireUnbreakable", function(ent, dmginfo)
if ent.wire_unbreakable then dmginfo:ScaleDamage(0) end
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if ent.wire_unbreakable then dmginfo:ScaleDamage(0) end
if ent.wire_unbreakable then return true end

if not ValidAction(self, this, "break") then return end
if this:GetClass() ~= "prop_physics" then return self:throw("This entity can not be made unbreakable!", nil) end

this.wire_unbreakable = this:Health() > 0 and breakable == 0 and 1 or 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
this.wire_unbreakable = this:Health() > 0 and breakable == 0 and 1 or 0
this.wire_unbreakable = this:Health() > 0 and breakable == 0 and true or nil

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.

4 participants