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

Add new Traceray natives #754

Merged
merged 5 commits into from Aug 13, 2018

Conversation

@SlidyBat
Copy link
Contributor

SlidyBat commented Jan 21, 2018

Related to #749
Adds TR_EnumerateEntities and TR_ClipRayToEntity natives, which will be very useful for having more control over tracerays. More specifically, this allows plugin developers to create rays that can hit trigger entities.

Here's an example of how using these natives would look like in a plugin:

#include <sourcemod>
#include <sdktools>

public void OnPluginStart()
{
	RegConsoleCmd( "sm_trigtest", Command_TriggerTest );
}

public Action Command_TriggerTest( int client, int args )
{
	float startpos[3];
	GetClientEyePosition( client, startpos );
	float angles[3];
	GetClientEyeAngles( client, angles );
	
	// native void TR_EnumerateEntities(const float pos[3], const float vec[3], bool triggers, RayType rtype, TraceEntityEnumerator enumerator, any data=0);
	TR_EnumerateEntities( startpos, angles, true, RayType_Infinite, HitTrigger );
}

// return true to continue enumerating, or false to stop
public bool HitTrigger( int entity )
{
	char classname[64];
	GetEntityClassname( entity, classname, sizeof( classname ) );	
	if( StrContains( classname, "trigger" ) > -1 )
	{
		// native void TR_ClipRayToEntity(int flags, int entity, Handle tr=INVALID_HANDLE);
		TR_ClipRayToEntity( MASK_ALL, entity );
		
		float pos[3];
		TR_GetEndPosition( pos );
		
		PrintToChatAll( "Hit a trigger! Collided at (%.2f, %.2f, %.2f)", pos[0], pos[1], pos[2] );
		return false;
	}
	
	return true;
}

Also I think the diffs looks weird because VS told me that line endings were inconsistent and it "fixed" them.
EDIT: Re-did it without the line ending bs to fix the cancer diff.

@Headline

This comment has been minimized.

Copy link
Member

Headline commented Jan 21, 2018

Can you go back and undo the white space changes so the diffs only contain what you’ve added? You’ve really nuked line endings here

@asherkin

This comment has been minimized.

Copy link
Member

asherkin commented Jan 28, 2018

Is TR_ClipRayToEntity ever useful outside of the TR_EnumerateEntities callback?

Does something like the following make more sense to be consistent with the other TR natives:

public Action Command_TriggerTest( int client, int args )
{
	float startpos[3];
	GetClientEyePosition( client, startpos );
	float angles[3];
	GetClientEyeAngles( client, angles );
	
	// native void TR_EnumerateEntities(const float pos[3], const float vec[3], bool triggers, RayType rtype, TraceEntityEnumerator enumerator, any data=0);
	TR_EnumerateEntities( startpos, angles, true, RayType_Infinite, HitTrigger );

	// TR_EnumerateEntities internally calls TR_ClipRayToEntity before returning when enumerator returns false.
	//TR_ClipRayToEntity( MASK_ALL, entity );
		
	float pos[3];
	TR_GetEndPosition( pos );
		
	PrintToChatAll( "Hit a trigger! Collided at (%.2f, %.2f, %.2f)", pos[0], pos[1], pos[2] );
}

// return true to continue enumerating, or false to stop
public bool HitTrigger( int entity )
{
	char classname[64];
	GetEntityClassname( entity, classname, sizeof( classname ) );	
	return StrContains( classname, "trigger" ) == -1;
}

This is making the assumption that after you've clipped the ray you'll always be returning false, is this assumption correct?

When is the mask param to TR_ClipRayToEntity needed? (other than MASK_ALL)

If TR_ClipRayToEntity is needed, I think it should probably be used with TR_GetEntityIndex, rather than inside the enumerator.

Only other thing I would like to see is replacing the boolean triggers param with a flag param - we're trying to move away from non-obvious boolean params as they make code harder to understand quickly.

@jason-e

This comment has been minimized.

Copy link
Contributor

jason-e commented Jan 28, 2018

(I had a different post previously but realized something at the last minute and have revised it)

I think the best solution is to call ClipRayToEntity automatically as you suggest (assuming it is not too wasteful if the user doesn't actually need it), and to also make it directly available, but with full ray parameters like this:

native void TR_ClipRayToEntity(const float pos[3], const float vec[3], int mask, RayType rtype, int entity);

This also makes the mask parameter make more sense.

I definitely think ClipRayToEntity should be available directly. I can envision scenarios where you have a known entity and just want to clip a ray to it, and having to go through EnumerateEntities would be odd.

At first I was against calling ClipRayToEntity automatically because it could potentially be wasteful to do so, but if we want to allow calling ClipRayToEntity on its own without it being tied to another TR_ call, then ClipRayToEntity needs its own ray parameters which would make it very awkward to manually call within or in response to the EnumerateEntities enumerator.

Edit: Also need to make sure ClipRayToEntity causes TR_DidHit to return true.

@SlidyBat

This comment has been minimized.

Copy link
Contributor Author

SlidyBat commented Jan 28, 2018

This is making the assumption that after you've clipped the ray you'll always be returning false, is this assumption correct?

Clipping the ray doesn't always mean that you want to stop enumerating. The condition for stopping the enumerating could be dependent on the results of ClipRayToEntity (or something else entirely).
e.g. You only want to stop enumerating if the entity position meets some requirements. I think it would be best if its left up to the people using these functions instead of taking away that option.

When is the mask param to TR_ClipRayToEntity needed? (other than MASK_ALL)

This depends on the use case. Using different masks on entities could give different results depending on where it hits them. For example, say I wanted to clip the ray to a players bounding box rather than it's hitbox, I could use MASK_PLAYERSOLID for that. I tested this out myself using this code. Here's an image showcasing it as well (red hits hitbox, green hits bbox): https://i.imgur.com/HjNZqEy.jpg

Only other thing I would like to see is replacing the boolean triggers param with a flag param

Do you mean making an enum specifically for this? I only made it a boolean since the function it calls also uses a bool so I thought that would be the most straightforward way to do it.

Also need to make sure ClipRayToEntity causes TR_DidHit to return true.

When the clipping doesn't hit anything, the trace result fraction is set to 1.0 and DidHit returns false

Also I wasn't sure about my ClipRayToEntity implementation, since it creates a new trace result out of the global ray. When you want to use the global trace result you pass in INVALID_HANDLE, or pass in a handle and it gets filled with a new trace result. Should I change this behaviour to work like the other TR functions with a regular/Ex version?

@SlidyBat

This comment has been minimized.

Copy link
Contributor Author

SlidyBat commented Feb 11, 2018

I've pushed another commit that seperates ClipRayToEntity into 2 groups of natives:

  • TR_ClipRayToEntity(Ex) - This now takes a start/end/raytype like the other Trace natives and initializes a new ray before calling ClipRayToEntity.
  • TR_ClipCurrRayToEntity(Ex) - This works like the implementation before this commit where it just performs ClipRayToEntity on the current global ray.

This way the ClipRayToEntity function isn't tied down to EnumerateEntities and can also be used by itself.
I also split those natives into regular/extended versions that return new trace results to make them more like the other natives.

Here is the same example plugin I posted above but updated to use the new natives: https://pastebin.com/CB5uJFLz

Any thoughts?

@jason-e

This comment has been minimized.

Copy link
Contributor

jason-e commented Feb 15, 2018

That implementation looks solid in my opinion.

Just to clarify why SildyBat went with two versions of ClipRayToEntity (not including their Ex counterparts):

At a minimum, it makes sense to have a version of ClipRayToEntity that mirrors the original (it accepts an arbitrary ray definition); but there is also potential for a need to call ClipRayToEntity as part of the EnumerateEntities enumerator - especially since the two tend to go together. Since we don't have access to the ray/trace instances in SourcePawn, having TR_ClipCurrRayToEntity makes that much easier. Without it we would need to pass something like a datapack to the enumerator and repeatedly initialize a new copy of the same ray.

@Pelipoika

This comment has been minimized.

Copy link
Contributor

Pelipoika commented Aug 13, 2018

Can we have this please?

@Kenzzer

This comment has been minimized.

Copy link
Contributor

Kenzzer commented Aug 13, 2018

Yeah please.

@asherkin asherkin self-requested a review Aug 13, 2018
@asherkin asherkin merged commit 144fb90 into alliedmodders:master Aug 13, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
sneak-it added a commit to sneak-it/sourcemod that referenced this pull request Sep 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.