Skip to content

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.

@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

@deltamolfar
Copy link
Contributor Author

Wait a bit before merging. I'm currently testing and also adding callback to remove unbreakable state on E2 removal

@deltamolfar
Copy link
Contributor Author

Done

@wrefgtzweve wrefgtzweve merged commit cef7a6c into wiremod:master Apr 28, 2025
1 check failed
@deltamolfar deltamolfar deleted the propUnbreakable branch August 29, 2025 19:06
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