🔨 Make id-space maths human readable/usable

[zach2good]

I affirm:

• [x] I understand that if I do not agree to the following points by completing the checkboxes my issue will be ignored.
• [x] I have read and understood the Contributing Guide and the Code of Conduct.
• [x] I have searched existing issues to see if the issue has already been opened, and I have checked the commit log to see if the issue has been resolved since my server was last updated.

Describe the feature

A vitally important bit of maths that we do in a lot of places is somewhat arcane and poorly explained/readable.

    auto id = 0x1000000 + (m_zone->GetID() << 12) + targid;

As we go from "WorldId" to "ZoneIndex" we do this specific transformation. This allows us to know what zone an entity is in based on its world ID, etc.

We also have the various "buckets" that different entities can live in (static, player, dynamic, etc.):

// Handles the generation and/or assignment of:
// - Index (targid)
// - Current Zone
// - Global ID (id)
// - Insertion into the zone's dynamicTargIds list
void CZoneEntities::AssignDynamicTargIDandLongID(CBaseEntity* PEntity)
{
    // NOTE: 0x0E (entity_update) entity updates are valid for 0 to 1023 and 1792 to 2303
    uint16 targid = 0x700;
    for (auto it : dynamicTargIds)
...

It would be super helpful if we extracted all of these out into constants and helper functions, and picked a naming system, and stuck with it. "targid" sucks. I like "id" and "index", but other additional descriptors are also welcome. Ashita tends to be very sane and aligned with what SE use internally, so it might be worth following their conventions for this.

[TeoTwawki]

I'm used to "long id" (includes zone id with entity id) and "short id" (Nth entity in current zone) myself. I think pxi (and then dsp) wound up with "targid" because some pointer to the player's current cursor target was using short IDs and then it just stuck in people minds at the time.

In windower addon land "index" is often not the one you'd expect - people have inconsistent use of that word. Less often but same is true for some ashita v3 addons.

In my own code I tend to use "id" to mean the long id since that is what I see in the dat file itself and polutils and mass extractor both call that "id" and that "identifies" the object globally, and I avoid using the word but still think of "index" to be "which one in this smaller list" for those short IDs specific to the zone. It is often clearest to just refer to them as long and short, as I just did in this post as it avoids anyone elses notions of which one they think is "index".

Several places in DSPs past began to refer to "longId" and "shortId" before being ripped out because someone mistakenly thought they would transition the entire codebase to using only short IDs before having that once more reversed. Seriously its been a not fun ride watching that flip back and forth over the years and things renamed in its wake.

[zach2good]

The issue I have with LongId and ShortId is that they're not particularly descriptive of what they are. Once I make these changes they're not going to be "just numbers", they're going to be typesafe and not immediately interchangeable.

Example from another project of mine (needs some work, but the idea is there):

struct NodeId { u64 value; };
struct GateId { u64 value; };

inline bool getNodeValue(NodeId id) ...

^ In this case, the only thing you can pass into getNodeValue without jumping through a bunch of hoops is a NodeId. A GateId isn't compatible, even though they're the same underlying type.

I like Id and Index, but even those could stand to be clearer about what they are. GlobalId and ZoneIndex maybe.

[TeoTwawki]

yeah ID and Index with no other qualifiers have both been loaded terms with different meanings to different people is exactly a problem I was alluding to.

I'm always wary of everything trying to be typesafe - its great at stopping misuse but not without its drawbacks. sometimes having "just a number" is an advantage. Probably fine, but one of those things I'd need to see first to know if I was going to have an issue with it. c23 brings in a feature that makes this less of a problem, where you can easily access the underlying type if you need to while remaining scope safe. And Scope limiting appeals to me much more than type limitations. So eventually my issues with type safety will go away on their own! (its be easy to just think "well he must not get the point of type safety" and you might even be right if you thought that on reading this, but I'm not wrong about it sometimes being a pain in the ass either 😉 )

[zach2good]

I'd rather the pain of over-the-top type safety and having to bend over backwards to do what I want/need than being able to shoot myself in the foot by putting number x where number y goes, just because they're both uint16 under the hood etc.

The part of the spec Teo is talking about: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3030.htm

[TeoTwawki]

thoughts on zoneEntityID and globalEntityID ? its more verbose but I dunno if we want to avoid "id" altogether.

[zach2good]

As types, ZoneEntityId and GlobalEntityId would do exactly what we need 👍

[UnculturedButter]

To add to this, if you want to move down the type safety path, have you considered going fully down that lane and moving to Strong Types?

The base case is something like

template <typename T, typename Parameter>
class NamedType
{
public:
    explicit NamedType(T const& value) : value_(value) {}

    template<typename T_ = T, typename = IsNotReference<T_>>
    explicit NamedType(T&& value) : value_(std::move(value)) {}

    T& get() { return value_; }
    T const& get() const {return value_; }

private:
    T value_;
};

This ends up really no different than the struct method mentioned above. But it can easily be improved by adding in all of the operator overloads. Expanding it fully to include those frees you from having to constantly access the storage member variable all the time and can allow you to write more readable code like

XCoordinate x{10};
YCoordinate y{20};

...

XCoordinate deltaX{49};

x +=deltaX ;

All always depends how deep down the type safety rabbit hole you want to go, and with good compiler optimization this stuff all the boilerplate gets thrown away at compile time but keeps you safe while writing. And somewhere down the line you'll get tired of constantly writing x.value + deltaX.value, so you'd consider plopping operators down somewhere and, since this is c++ that may as well be in a class

[zach2good]

Yeah, I had this snippet written down very similar to yours, using CRTP:

template <class T>
class NumericBase
{
    T& operator+=(const T& rhs)
    {
        data += rhs.data;
        return static_cast<T&>(*this);
    }

    friend T operator+(T lhs, const T& rhs) 
    {
        lhs += rhs; 
        return lhs;
    }

private:
    int data;
};

class GlobalId : public NumericBase<GlobalId>{};
class LocalId : public NumericBase<LocalId>{};

int main()
{
    GlobalId gId0, gId1;
    LocalId lId0, lId1;

    auto gId2 = gId0 + gId1; // Allowed
    auto bad = gId0 + lId0; // Not allowed

    return 0;
}

We use a system very similar to this at my work to make sure everything is super type safe and its very difficult to mix up coordinate systems. Same ideas apply here.

I did notice recently that we have this struct:

struct EntityID_t
{
    void clean()
    {
        id     = 0;
        targid = 0;
    }

    uint32 id;
    uint16 targid;
};

Which isn't typesafe, and doesn't do the conversions between id/index, but might show that keeping them together instead of apart might be useful. An index is never useful without the context of the zone that it's in, and the global id can always be broken down to its zone and index, so it might be worth keeping them together at all times.

[UnculturedButter]

Yeah, very similar.

My work uses the StrongType idea with a little macro and CRTP to add in attributes like your NumericBase but in more sweeping terms like using CStrongTypeWithAttributes(TimestampInMilliseconds_t, Uint32_t, NStongtypeAttributes::Comparable, NStrongTypeAttributes::BasicArithmetic);

Where NStongtypeAttributes::Comparable implements the all of the <.><=,>=,== operators and BasicArithmetic implements + , - , , / , += , -= , =, /=, and % operators.

We also use an ImplicitlyConvertableTo<Type> flag that lets us easily use our strong types with other areas of code or third party libraries that don't support them that converts it to the underlying type automatically if needed. That opens up a little bit of danger type safety wise and you can technically assign something like our TimestampInMilliseconds_t to any Uint32_t, but it's a give and take on safety vs ease