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

E2 Collision Core(now part of propcore) #3099

Merged
merged 14 commits into from
Jul 22, 2024

Conversation

DerelictDrone
Copy link
Member

@DerelictDrone DerelictDrone commented Jul 16, 2024

Adds entity collision tracking for E2s.

Collision type (xcd) Has the contents and naming scheme from the Gmod struct of the same name, but without the PhysObj's
Functions table collision:toTable()
Returns a table with the key names and values being the same as the CollisionData struct(minus the physobjs)
Getters

Vectors

HitPos

  • collisiondata:pos()
  • collisiondata:hitpos()
  • collisiondata:position()

OurOldVelocity

  • collisiondata:ouroldvelocity()
  • collisiondata:entityoldvelocity()

TheirOldVelocity

  • collisiondata:ouroldvelocity()
  • collisiondata:hitentityoldvelocity()

HitNormal

  • collisiondata:hitnormal()

HitSpeed

  • collisiondata:hitspeed()

OurNewVelocity

  • collisiondata:ournewvelocity()
  • collisiondata:entitynewvelocity()

TheirNewVelocity

  • collisiondata:theirnewvelocity()
  • collisiondata:hitentitynewvelocity()

OurOldAngularVelocity

  • collisiondata:ouroldangularvelocity()
  • collisiondata:entityoldangularvelocity()

TheirOldAngularVelocity

  • collisiondata:ouroldangularvelocity()
  • collisiondata:hitentityoldangularvelocity()

Entity

HitEntity - collisiondata:hitentity()

Number

Speed - collisiondata:speed()

DeltaTime

  • collisiondata:deltatime()

OurSurfaceProps

  • collisiondata:oursurfaceprops()
  • collisiondata:entitysurfaceprops()

TheirSurfaceProps

  • collisiondata:theirsurfaceprops()
  • collisiondata:hitentitysurfaceprops()

Adds event entityCollision(Entity:entity, HitEntity:entity, CollisionData:collision)
Called when a tracked entity collides with another entity(does not trigger on hit world)

Tracking function applies the PhysCollide and CallOnRemove callbacks to an entity if it's valid, cleans up on chip destruct, if tracking is stopped manually, or ent is deleted.

Functions number trackCollision(entity ent)
Returns 1 when tracking is successfully started, returns 0 if already tracking this ent or failed to track. Needs event entityCollision

number trackCollision(entity ent, function cb(eexcd) )
Calls callback on collision, then fires the event entityCollision afterward, same functionality as trackCollision but doesn't need event entityCollision to exist

number isTrackingCollision(entity ent)
Returns 1 if ent is being tracked by this chip, 0 if not or if ent is invalid

void stopTrackingCollision(entity ent)
Stops tracking ent, removes callbacks from ent

Open to suggestions

@DerelictDrone DerelictDrone requested a review from Denneisk July 16, 2024 22:24
@deltamolfar
Copy link
Contributor

deltamolfar commented Jul 17, 2024

Nice job, but it should be moved to propcore imo, to lesser the sheer amount of vanilla cores, and due to collision tracking having little sense outside of prop manipulation scope.

I would rename their/our prefixes to something more general. Perhaps hitentity/entity?

Would be also nice to have functions throw strict-only errors on attempting to use invalid entity/track already tracked entity, etc, to make more use out of strict mode, and make debuggin for people easier

@DerelictDrone
Copy link
Member Author

Nice job, but it should be moved to propcore imo, to lesser the sheer amount of vanilla cores, and due to collision tracking having little sense outside of prop manipulation scope.

Moved to propcore

I would rename their/our prefixes to something more general. Perhaps hitentity/entity?

Made aliases for our/their getters as entity/hitentity for them, but keeping the original our/theirs around

Would be also nice to have functions throw strict-only errors on attempting to use invalid entity/track already tracked entity, etc, to make more use out of strict mode, and make debuggin for people easier

The 1/0's for the functions now produce runtime errors instead, or in non-strict return 1/0 for success/fail, and trying to check if we're tracking an invalid ent now produces an error too in strict instead

@DerelictDrone DerelictDrone changed the title E2 Collision Core E2 Collision Core(now part of propcore) Jul 17, 2024
@deltamolfar
Copy link
Contributor

Seems good enough
(Soon sf will have no advantages over E2 >:D)

@unknao
Copy link
Contributor

unknao commented Jul 17, 2024

I think this would be a great addition to expression 2, but i would also like to see a way to manipulate what collision group an entity belongs to like the glua function Entity:SetCollisionGroup would allow, maybe even Entity:SetOwner.

@deltamolfar
Copy link
Contributor

@unknao Collision group functionality exists in e2 for ~half a year already :D.
And setowner will bring more harm then good imo

@CornerPin
Copy link
Contributor

CornerPin commented Jul 17, 2024

It would be great if you could add E2Helper descriptions and convert the collision method names to camel case to be consistent with the rest of the E2 functions.

Here are some issues I've found:

  • GetHitPos will always throw an error, it seems that you forgot to change this to self.
  • Vectors returned by collision methods should be copied to prevent players from modifying them in other chips and addons. Example:
@name Chip1
@strict

if (first()) {
    ToTrack = noentity()
}

if (clk("track")) {
    trackCollision(ToTrack)
}

event entityCreated(Ent:entity) {
    if (Ent:type() == "prop_physics" & Ent:owner() == owner()) {
        ToTrack = Ent
        
        # A delay is needed to order the collision callbacks so that this
        # chip's callback always executes after the vector has been modified.
        timer("track", 0)
    }
}

event entityCollision([_ _]:entity, Coll:collision) {
    print("E2: " + Coll:ouroldvelocity())
}
@name Chip2
@strict

event entityCreated(Ent:entity) {
    if (Ent:type() == "prop_physics" & Ent:owner() == owner()) {
        trackCollision(Ent)
    }
}

event entityCollision([_ _]:entity, Coll:collision) {
    const Vec = Coll:ouroldvelocity() # Or: Coll:toTable()["OurOldVelocity", vector]
    Vec[1] = 1
    Vec[2] = 2
    Vec[3] = 3
}
hook.Add("PlayerSpawnedProp", "e2test", function(_, _, ent)
    timer.Simple(0, function()
        if not ent:IsValid() then return end
        
        ent:AddCallback("PhysicsCollide", function(_, data)
            print("Lua", data.OurOldVelocity)
        end)
    end)
end)

If you run these scripts and spawn a prop, you will get this output:

Lua	1.000000 2.000000 3.000000
E2: vec(1,2,3)

You can prevent this by copying vectors using the vector constructor before passing them to E2.

@DerelictDrone
Copy link
Member Author

DerelictDrone commented Jul 17, 2024

It would be great if you could add E2Helper descriptions and convert the collision method names to camel case to be consistent with the rest of the E2 functions.

E2Helper descriptions have been written

  • GetHitPos will always throw an error, it seems that you forgot to change this to self.

In this particular case I missed changing 'this' to' collision', 'self' refers to the e2 chip and 'this' refers to the object that the :function() was called from.

  • Vectors returned by collision methods should be copied to prevent players from modifying them in other chips and addons.

Thank you for bringing this to my attention especially though, as I was under the impression initially that the physcollide callback wouldn't pass the same table(or contents) to every callback func

Comment on lines 1465 to 1467
function( us, cd )
chip:ExecuteEvent("entityCollision",{us,cd.HitEntity,cd})
end)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure this function is executed in the physics engine context which can allow user code to break the physics engine by doing weird stuff inside of this hook. Collisions should be stored in a table and then execute the E2 via a think hook, similar to what SF does.

Copy link
Member Author

Choose a reason for hiding this comment

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

So would I want to set something up like E2's tick hook to run the event on the chip for each queued collision?

Copy link
Contributor

Choose a reason for hiding this comment

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

What the heck is that timer there for?
Yeah, create a tick hook when collisions get queued and remove it after it runs.

Copy link
Contributor

Choose a reason for hiding this comment

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

What the heck is that timer there for?

Copy link
Member Author

Choose a reason for hiding this comment

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

What the heck is that timer there for?

The timer there goes back to 2009(pre-github presumably), the last time it was touched was for style standardization in 2013, so I don't know either.

@wrefgtzweve
Copy link
Contributor

Would be nice if this could be its own core for more fine-grained control

@deltamolfar
Copy link
Contributor

deltamolfar commented Jul 18, 2024

@wrefgtzweve He made it in it's own core at first, but then moved to propcore. I guess, again, it would be alright if there would be just a convar to disable it, as moving it to complete separate brand new core will just bloat the sheer amount of vanilla cores even more, while also adding a core that have only few functions, single event, and have little to no application if the propcore is disabled. (Nor possibility that this core will grow, as it already does everything its suppose to)

What I would want to test - will this make a new way to spam network? For example you start tracking 100 ents, and then collide hell out of them? If the network do raise significantly - I think silly limit (like 25 ents tracked at the same time) would be sufficient.

@wrefgtzweve
Copy link
Contributor

the entire point of cores is to have fine grained control over them
having a convar for something like this seems redundant when cores already exist.

i dont see how networking could be an issue here, it seems all serversided unless i missed something.

my main concern is that a player adds a lot or a few laggy running collision listeners, this will currently lag the server but not add to cputime/ops.

@DerelictDrone DerelictDrone requested a review from thegrb93 July 18, 2024 17:43
Comment on lines 1553 to 1554
hook.Add("Think", "Expression2CollisionClock", E2CollisionEventHandler)
timer.Create("Expression2CollisionClock", 5, 0, function()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this timer exist. You should add the think hook when a collision is queued and remove it after it runs.

Copy link
Member Author

Choose a reason for hiding this comment

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

So like, the function I have right now is fine, just have it added on a collision and have it hook.remove itself after run?
Or do you want a unique hook generated for each chip / collision to run the event once for that single event?

Is there a big cost to hook.add overwriting a pre-existing hook that might make it worth storing a bool for the hooks existence locally to check against?

On the matter of the timer, I figured it was a failsafe for the e2 tick hook getting cleared during load but if we shift away from the check being every think to just the think after a collision the timer can definitely go.

Copy link
Contributor

Choose a reason for hiding this comment

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

Even better is

if not var then
    var = true
    timer.Simple(0,function()
        processCollisions()
        var = false
    end)
end

@DerelictDrone DerelictDrone requested a review from thegrb93 July 18, 2024 23:34
@deltamolfar
Copy link
Contributor

deltamolfar commented Jul 19, 2024

Regarding core/convar convo:
Imo - cores are chunk control. You either want all/most of it, or disable it altogether, and as such, having little cores have little sense to me personally (apart for things like http, which are small by size, but are way too different to be placed in any existing core), and convars are the fine grained control we're talking about. But agree to disagree I guess :).

Regarding possible lag:
Someone should stress test it, before merging

@Denneisk
Copy link
Member

It would be a lot cleaner to use function callbacks instead of events.

@thegrb93
Copy link
Contributor

Do you have code example for that?

@DerelictDrone
Copy link
Member Author

It would be a lot cleaner to use function callbacks instead of events.

I could probably make an overload for trackCollision that'll store a passed lambda and call it instead of executing the event for that entity, if that's what you mean

@deltamolfar
Copy link
Contributor

I also thought of that, but that is not in style if E2 to use callback functions as arguments at all. A lot of people will not understand this concept, and thus will not use it. (Most of e2 users are not irl programmers)

I guess an additional function with callback could be done, but not by not firing event, as it will break the current events expected behavior. Also no need to not execute event, as people that would want and know how to use callbacks will understand how not to get rekt by both callback and event.

return self:throw("Attempting to track collisions for an invalid entity",0)
end

e2function number trackCollision( entity ent, function cb )
Copy link
Member Author

Choose a reason for hiding this comment

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

This is going to break glualint but it's valid & works ingame, fairly certain this may be the only vanilla e2 function(not automatically generated) that takes a lambda at present.

@DerelictDrone
Copy link
Member Author

DerelictDrone commented Jul 20, 2024

May not be the most scientific stress test but here's two recordings of the same save, one with an e2 set up to count ALL collisions and one without.

E2 code used:

@outputs Collisions:number Tracked:number
@strict

Collisions = 0
Tracked = 0

event entityCreated(Ent:entity) {
    if(Ent:isValid()) {
        if(!isTrackingCollision(Ent)) {
            trackCollision(Ent)
            Tracked++
        }
    }
}

event entityCollision(Entity:entity, HitEntity:entity, CollisionData:collision) {
    Collisions++
}
e2.tracking.mp4
no.e2.tracking.mp4

Edit: Here's three saves, 1 has no e2, 2 has 20 that display the number of collisions & tracked entities, and 3 has 20 that display & have a potentially expensive sqrt while loop

@deltamolfar
Copy link
Contributor

Seems fine to me

@thegrb93 thegrb93 merged commit c8f38c3 into wiremod:master Jul 22, 2024
1 check failed
@DerelictDrone DerelictDrone deleted the collision-core branch July 23, 2024 02:50
if self.data.E2TrackedCollisions[entIndex] then
return self:throw("Attempting to track collisions for an already tracked entity",0)
end
-- First, double check the arg sig lines up
Copy link
Contributor

@Vurv78 Vurv78 Aug 6, 2024

Choose a reason for hiding this comment

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

Just FYI, This is what Function:Unwrap is for, in case you didn't know. Although you used other Function: Methods, so you probably did. Maybe a separate method for just checking the arguments would suffice.

function Function:Unwrap(arg_sig, ctx)

Copy link
Member Author

@DerelictDrone DerelictDrone Aug 6, 2024

Choose a reason for hiding this comment

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

I had overlooked unwrap, it would be nice if it could be passed an optional format/prepended string though so I can use the wording of the error message here but if you don't think that's needed I'll get a PR up soon to replace this check and msg with the unwrap

Copy link
Contributor

@Vurv78 Vurv78 left a comment

Choose a reason for hiding this comment

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

,

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.

8 participants