Trace builder API proposal

[slipher]

I propose using a builder interface for traces. For example instead of

// Example 1
trace_t tr;
trap_Trace( &tr, origin, vec3_origin, vec3_origin, dest, ENTITYNUM_NONE, MASK_SOLID, 0 );

// Example 2
trace_t tr;
trap_Trace( &tr, ent->r.currentOrigin, ent->r.mins, ent->r.maxs, origin, ent->num(), ent->clipmask, 0 );

We would have instead

// Example 1
trace_t tr = TraceBuilder(origin, dest, MASK_SOLID).Trace1();

// Example 2
trace_t tr = TraceBuilder(ent->r.currentOrigin, origin, ent->clipmask)
    .bounds(ent->r.mins, ent->r.maxs)
    .passEntity(ent->num())
    .Trace1();

The full API would look like this. vec3_t variants are omitted for brevity.

class TraceBuilder {
public:
    /*
     *  Start/end and content mask are required, and passed in the constructor
     */

    // traces along the line from start to end
    TraceBuilder(const glm::vec3& start, const glm::vec3& end, int contentMask);

    // position test (start == end)
    TraceBuilder(const glm::vec3& position, int contentMask);

    /*
     *  Optional trace parameters
     */

    // set box dimensions (default is a point)
    TraceBuilder& bounds(const glm::vec3& mins, const glm::vec3& maxs);

    // Set one entity to ignore
    TraceBuilder& passEntity(int entityNum);
    TraceBuilder& passEntity(gentity_t* ent);

    // don't collide with objects matching these content flags
    TraceBuilder& skipContents(int skipMask);

    /*
     * Get the results
     * Note: Trace1 and Trace2 are equivalent for position tests
     */

    // Trace with traditional startsolid semantics
    // (objects overlapping the start position may be ignored)
    trace_t Trace1();

    // Trace with startsolid => collision semantics
    trace2_t Trace2();
};

What do you think? An alternative would be to set the start/end and content masks using builder methods, instead of from the constructor. In this case I would add booleans to track whether those required parameters are set and call Sys::Drop if they are not.

Don't worry about unfinished migrations: if I were to merge this, I would migrate all the callers in sgame.

[DolceTriade]

I think using a builder paradigm is sensible.

I would rename some of the methods tho:

First of all, I would avoid mixing lower case methods with upper case methods. I would stick to using Upper case for all methods, especially if they are public methods.

passEntity -> ignoreEntity skipContents -> ignoreContents

Trace1 -> TraceIgnoreStart() Trace2 -> Trace

An alternative API might be something like

trace_t tr = TraceBuilder(ent->r.currentOrigin, origin, ent->clipmask)
    .bounds(ent->r.mins, ent->r.maxs)
    .passEntity(ent->num())
    .IgnoreStart(false)
    .Trace();

[slipher]

That's a good idea to make the startsolid behavior one of the build-able parameters. I didn't think of it because the two trace functions return different types, but we could just erase the small amount of extra information, which is almost never used, from trace_t and always return trace2_t. I can only spot one place where trace_t::startsolid is used in sgame, in G_Physics. But it doesn't even make any sense to me how it's being used.