stop using `float[2]`, `float[3]`... in the public interface

[ghost]

I have a modified unvanquished codebase which almost got rid of the C Vector API, replaced by glm library, but I'm hitting daemon's limitations as cgame interacts directly a lot more with the engine than sgame.

I originally though it would be a good idea to just rely on exposing glm in the public interface, but thinking more about it, this might not be such a good idea, as in theory one might be willing to switch to a different lib someday (or even today, as glm as yet to go in daemon).

One of the major issues with the current API is that it defines arrays, which means (pointers to) arrays are passed all around. Those can be (and are!) randomly abused to pass optional values, without anything in the signature or "documentation" making it obvious that the array is meant to be an actual mandatory value (a reference, in C++), an optional value (*), or the start of an array of data... Other issues are:

• almost (to be optimistic) no documentation
• macro based
• requires out parameters, which implies declaring variables before calling the functions, preventing code like this: vec3_t angles = RotatePointAroundVector( vec3_t const& dir, vec3_t const& point, float degrees );
• C-stuff, makes simple maths expressions hard to read: you can't do vec3_t destination = start + direction * distance;

All the stuff I'll discuss also apply to vec2_t, vec4_t, mat3_t, etc.

I intend to finish the work of getting rid of the Vector API in my gamelogic work very soon (in February, to be exact) but as said on IRC, I can see multiple ways of doing it, some would bring more benefits to everyone, others would be completely independent and hence would not benefit anyone but my code:

• migrate daemon's public API to glm:

  • no compat layer for Unvanquished as it is moving toward glm
  • introduces a 3rd party dependency on the public interface, that I tend to consider a rather bad thing for long-term compatibility, but this would still be better than current situation, as glm at least is fully documented and maintained
  • this benefits everyone
  • probably bulky, will introduce lot of changes to review, which imply long discussions that will bore everyone, not to mention the code reviewing
  • breaks API compatibility but probably keeps ABI compatibility with gamelogic

• implement an alternative public API using glm

  • no compat layer for Unvanquished as it is moving toward glm
  • introduces a dependency on the public interface
  • benefits everyone
  • can be merged in atomic chunks, easier to review
  • can be optional, driven by a compilation flag for example, to allow running both experimental and stable codebases
  • breaks API compatibility but probably keeps ABI compatibility with gamelogic

• migrate daemon's public API to something like union vec3_t { float d[3]; struct { float x, y, z; } xyz; struct { float r, g, b; } rgb };

  • requires compat layer when gamelogic wants to use a 3rd party library, but C++ allows to easily implement cast operators (which is not true for the current typedef stuff)
  • no external dependency
  • this benefits everyone
  • breaks API compatibility but probably keeps ABI compatibility with gamelogic
  • can be implemented with a bunch of trivial, automated changes
  • suffers the lack of documentation and maintenance problems of current API

• gamelogic can implement some hackish proxy that no longer include daemon's definition for structures like playerState_t, refdef_t, etc

  • benefits only gamelogic that does it
  • no need for review or talking as won't be merged
  • no API/ABI breaks
  • ugly and hackish

Those are the 4 solutions I see. I'll rely on the hackish solution for myself if the discussion goes nowhere or the end decisions would require too much additional work from me: as I said, I intend to complete the glm migration of my codebase next month. To give you an idea about how many work I still have to do, grep can only find 98 occurrences of vec3_t in my codebase, so yeah, I'm quite serious about finishing this stuff soon.

*: in C, there was only 2 choices: use pointers, or implement a function with a proper name. C++ have two more options: default values (that I dislike because they make code more confusing), and using a different signature with the same function name. In daemon or unvanquished, notable examples of better function name are the trace functions, which can be either about RayTracing (no box sizes) or about BoxTracing (with the dimensions of the box to swipe around the world). There are other occurrences, but this is out-of-topic.

Imo where pointers can be useful in C++, but C++ also have default mechanisms - that I personally dislike - and even C allows to just define a function with a name describing the

[DolceTriade]

Is using &glmVec[0] too hackish?

[ghost]

That's what is currently done in unvanquished. This suppose that glm::vec3 have the exact same memory layout and byte size as vec3_t, notably. It's also "compatible" with any kind of typedef float[XX] vecXX_; (or matXX_t or quatXX_t, ofc) which means it's quite fragile and might break without warning when something changes either in glm, in daemon-engine, or in a game based on daemon-engine (changes which have various probabilities to happen, pretty low in most cases I'd say, but still). Conclusion: this is a hack that work by luck, but I'd rather not rely on this in future, especially since this hack is used across the whole gameplay codebase, so a breaking would imply changing a lot of lines. Currently, I count 112 lines matching this pattern in my gameplay codebase. Each of those is susceptible to break, in non-explicit ways, and to require patching when this happens.

[DolceTriade]

I'm not sure how beneficial it would be to bring glm into daemon... I think implementing some direct proxy between vec and glm at the public interface is probably the approach I would go for (which seems to be what you are doing)

[ghost]

which seems to be what you are doing

That's the whole point of this ticket: I'm not doing anything yet. I want upstream to choose a solution before I decide what I'll spend my time into, so that there will be less discussions when the time comes to do a PR. I will do something whatever happens, as said, and this might, or not, benefit upstream depending on the discussion here.

[necessarily-equal]

What do you have in mind as “interface struct”?

Something like this?

struct vec3 {
    float x, y, z;
    operator[](size_t i) { return (&x)[i]; } // for things such as `v[2]`
    float *() { return &x; } // implicit cast operator
    const float*() const { return &x; }
};

Do you think having an implicit cast to/from glm::vec3 would be a good idea? On one side, glm::vec3 is a simple struct without virtual members so it should be fine™, we can add an assertion that it stays POD and I mean, it is faster (zero cost). On the other side, it's a bit hacky.

[ghost]

Yes, that's the idea. Except that I would do something like struct vec3_t { float d[3]; }; instead, so that converting would be done with relatively boring changes (possibly good parts would be doable with a regex).

Do you think having an implicit cast to/from glm::vec3 would be a good idea?

I think it might not be a good idea to have glm in the public interface anyway. Also, all of glm, daemon and gamelogic requires C++, and C++ does provide cast operators, so I do not see any problem. The cast can be implicit or explicit, this is for the gamelogic to decide, not daemon's problem. Implicit casts in gamelogic are currently legion, anyway.

On the other side, it's a bit hacky

As long as it's not reinterpret_cast or const_cast I do not see why this would be hacky.

The alternatives are to keep the &foo[0] and VEC2GLM hacks in gamelogic, which require temporary copies in several situations, or to use glm directly on the public interface, either by providing a new public interface, or by moving the whole public interface in one shot.

The first and current situation is much more hacky than the struct solution, and I'd rather do something really ugly that sits at only one place (so changes and fixes happen in a single place, instead of through all the codebase), than to keep this half-ass "solution". The later solution, glm in public interface, will require a lot more work while also forcing gamelogic to depend on glm. More work in both variants of the solution, and one of those variants would even break API compat with gamelogic, which I'd say should be a big no.

And of course, keep in mind that when I say "more work", this means both:

• more work to implement;
• and more work to review, with all the problem this tends to create;

Considering the manpower available, I don't think this would be wise.

[slipher]

I don't think the current state of the interface is too terribly bad. Using float* as the interface at least has the advantage that it can be compatible with pointing into the arrays of different float libraries. The lack of value types to use as return values is perhaps the main disadvantage.

For a little more type safety for functions taking a vector argument, we could consider a class similar to StringRef but for vectors which could improve safety by preventing using a vector of the wrong length or a null pointer. Something like

class Vec3Ref {
    private: 
    vec_t* data_;

    public:
    Vec3Ref(vec_t(&arr)[3]): data_(arr) {}
    
    template<size_t N>
    Vec3Ref(vec_t(&)[N]) = delete; // error on wrong array size
    
    // only explicit conversion from pointer
    explicit Vec3Ref(vec_t* p): data_(p) {}
    
    vec_t& operator[](size_t i) {
        ASSERT_LT(i, 3);
        return data_[i];
    }
};

...

// defined in gamelogic
vec3_t& ToVecRef(glm::vec3& v)
{ 
	// this is technically undefined behavior as is any use of &v[0] from a glm vector
	// since is declared like float x, y, z;  not float data[3]
	return reinterpret_cast<vec3_t&>( v[0] );
}

[illwieckz]

I would personally encourage a port to GLM because the less we have custom math functions, the better we are.

I don't mind the library, being GLM or others, but since we already use GLM in game code, using GLM in engine would be a good choice to avoid library proliferation.

[illwieckz]

@bmorel, can you fill this table by saying what is the 🥇️ more convenient solution for you when contributing to Unvanquished code base on short term, mid term and long term? Then mark the 🥈️ secondary more convenient solution in remaining cells.

There can only be one primary 🥇️ per column, but there can be any 🥈️ secondary per line. Cells can be empty, especially if it's bad.

Proposition	Short term	Mid term	Long term
migrate daemon's public API to glm
implement an alternative public API using glm
migrate daemon's public API to something
gamelogic can implement some hackish proxy

Here is an example I quickly did (it's not yet my opinion):

Proposition	Short term	Mid term	Long term
migrate daemon's public API to glm		🥇️	🥈️
implement an alternative public API using glm	🥈️	🥈️	🥇️
migrate daemon's public API to something	🥈️	🥈️
gamelogic can implement some hackish proxy	🥇️

I want to add something anyway: if we port Dæmon to GLM, we would probably move the GLM submodule from Unvanquished to Dæmon to avoid having two GLM submodules. It means both game and engine code would rely on the exact same version of the GLM API. It would mean the cost of keeping the game and the engine compatible with the same GLM API version would be the exact same cost, whatever we use GLM as a public interface between game and engine, or we use GLM in both game and engine with a neutral interface in between.

As a side question, would you complete the same table for when porting an hypothetical existing game like another Q3 mod (let's imagine Smokin'Guns for example). Example of question: What would you think would be easier for such another project, between porting such other game to GLM as an interface, or porting it to the neutral layer as an interface. Those answers and this table doesn't contribute to the above table (that's why I said when contributing to Unvanquished code base for the first table).

[ghost]

well, if you avoided using stupid emoticons, yes, I would have filled your arrays. Now, no, I can't. I only have a laptop, had to buy an alim for the week, 25€ wasted because of me.

Also, I think I made it clear how I would fill that "array".

[ghost]

I would appreciate explanations about what is not clear in descriptions, really. I would also appreciate to have upstream to take a decision, which is why I let the question this open. My hope is that my fork will not have good reasons to last.

[ghost]

Basically, my intent is to prove that unvanquished could be much better that what it currently is, and for various reasons I think this can't be achieved by working together, which is sad. If I'm wrong, then it's fine. BUt I believe unv lacks a leader and that this lack is used to make unv stall

[illwieckz]

Use 1 and 2 if you want, this has no implication on meaning. I'm giving you the power to tell what is better for you. From those four options, what are the ones that convince you.

[ghost]

Migrate daemon's public API to glm => the cleanest. Requires lot of work, including syncing between engine and gamelogic. implement an alternative public API using glm => the easiest. Does not requires any syncing with gamelogic. migrate daemon's public API to something => probably the most consensual. Requires syncing with gamelogic, but most if not all of the changes will be trivial, could even be automated if someone knows how to run coccinelle on a C++ codebase (I don't). gamelogic can implement some hackish proxy => bad. Last resort "solution". To only use if no decision is taken. The least immediate work, by far, but I would really, really prefer to avoid this.

From a gamelogic's poing of view, the one requiring the least work is "implement an alternative public API using glm" since gamelogic is going toward glm. The 2nd one would be "migrate daemon's public API to something". From an engine's PoV, "migrate daemon's public API to something" is the one requiring the least work. I am unsure which one requires the least work overall. Probably struct vec3_t { float d[3]; }; as this is very compatible with the current codebase.

[illwieckz]

Sorry for the late answer.

This is how I would sort the given propositions given what I understand from them and from your expressed opinion about them:

Proposition	Short term	Mid term	Long term
migrate daemon's public API to glm			🥇️
implement an alternative public API using glm	🥇️	🥇️	🥈️
migrate daemon's public API to something	🥈️	🥈️	🥈️
gamelogic can implement some hackish proxy	🥈️

It looks like the two acceptable options would be:

• implement an alternative public API using glm
• migrate daemon's public API to something

I guess that if we go the first route, once everything from gamecode is ported to the new API, the result is the same of the second one. Also I assume that in the future migrating from implement an alternative public API using glm to migrate daemon's public API to glm would be even doable with less work than today if someone wants to do it.

So, I guess we can go the implement an alternative public API using glm way. The only real thing against it that it would introduces yet another new API, but I'm OK with the risk.

Though, I'm not a C++ master wizard, so I would like a comment by @bmorel where you're expressing your opinion on @slipher's comment above to make sure I miss nothing.

For example I wonder if that sentence means his suggestion would be good for porting random mods or games:

Using float* as the interface at least has the advantage that it can be compatible with pointing into the arrays of different float libraries.

I'm interested in good type safety and compiler awareness anyway.

[illwieckz]

If we go the implement an alternative public API using glm route, we may want to get rid of Vec API first so we exchange one unfinished API with another to-be-finished one so we would actually not proliferate on “alternative work-in-progress API”:

• https://github.com/DaemonEngine/Daemon/issues/787

[ghost]

we may want to get rid of Vec API first

Yes, before any solution, the deprecated new VecX API should be removed. Imo it's better to replace it with vecX_t API before as this will ease mass editing of the code (I think it's possible to do a bulk regex replacement to get rid of most of current API).