Mess with basic types

[D-mo]

There are several basic types in Orbiter that are defined in multiple places. For, example:

- OrbiterSDK defines: FVECTOR2, FVECTOR3, FVECTOR4 and FMATRIX4 in DrawAPI.h. Plus, VECTOR3, VECTOR4, MATRIX3 and MATRIX4 all with double data type in OrbiterAPI.h.

- Orbiter itself defines: Vector (3 double elements), Vector4 (double), Matrix (3x3 double), Matrix4 (4x4 double) in Vecmat.h. And, VECTOR2D is D3D7Util.h.

- Then, the DX7 client has a duplicate file D3D7Util.h which also defines VECTOR2D.

IMHO all these types should consolidated and live in one place, where they are accessible to the add-ons and the main Orbiter code. I am guessing they should be defined in a separate file in the OrbiterSDK/include dir, since it is included in both the SDK and the main code.

But, I am not familiar enough with Orbiter's code base and would like to get some input from other developers.

 

[n72.75]

Oh wow. Yeah we should probably only define those once. Maybe some kind of OrbiterTypes.h ?

I don't believe those have always been part of the API. NASSP has its own (very similar) version of the vector and matrix types mixed in with the oapi ones. We've removed some where we can.

Any addon devs from the 2003-5era remember anything noteworthy here.

I would say as long as it doesn't affect the use of API types and functions from the dev perspective it should be fine.

 

[GLS]

It may be important to know why the duplication exists.

 

[Max-Q]

Just don't break every module already compiled for Orbiter.

 

[DaveS]

Just don't break every module already compiled for Orbiter.

Speaking as someone who has used Orbiter since January 2002 and been an official Beta Tester since mid-2003 and released my first simple add-on in mid-2002 before that, unfortunately add-on breakage with a new version isn't anything new. Backwards compatibility just wasn't a thing back then. Just had to hope that the original dev was still around to update the add-on with closed sources and everything.

 

[Max-Q]

Yeah, but why break addons when you don't have to?
A lot of stuff from o2006 still works in 2010, and some 2010 stuff (Mostly MFDs and plugins) works in 2016.
We're talking about ZERO backward compatibility here.
There are many excellent addons for 2016 that work in OpenOrbiter now, but their devs aren't around anymore. All of this would be lost.
Obviously the move to 64 bit will do this as well, but that is for a good reason. Why not wait on this until then and do this when the 64 bit switch is made so we only have to break everything once?

 

[DaveS]

Yeah, but why break addons when you don't have to?
A lot of stuff from o2006 still works in 2010, and some 2010 stuff (Mostly MFDs and plugins) works in 2016.
We're talking about ZERO backward compatibility here.
There are many excellent addons for 2016 that work in OpenOrbiter now, but their devs aren't around anymore. All of this would be lost.
Obviously the move to 64 bit will do this as well, but that is for a good reason. Why not wait on this until then and do this when the 64 bit switch is made so we only have to break everything once?

Why delay the inevitable? It is going to happen. To use an actual spaceflight analogy: Back in the late 1990's, the Hubble Space Telescope detected a latent critical flaw in the the telescope's Power Control Unit (PCU). The PCU controlled all of the electrical flow to and from the batteries that actually powered the whole thing. After much discussion it was decided to Remove&Replace (R&R) the existing unit with a spare ground unit on the next Service Mission (SM). But before that could happen, all of the critical gyros on the telescope ended up failing in 1999 forcing a cut-down SM to fly in its place (STS-103) to replace the gyros to restore pointing and scientific capability.

The reason why there was much discussion about the existing the PCU was that in order to replace it, the entire telescope would have to be powered down completely, effectively killing it. And they had a very critical thermal clock ticking then as the longer the telescope was unpowered, the colder and closer to failure all of the electronics came. And there was a 0% guarantee that the telescope would power back up afterwards. Complicating things was that the PCU was not meant to be replaced and had no EVA-friendly connectors which forced the EV in question (John Grunsfeld) to disconnect them in the blind and then reconnect them again once again the blind (connectors were on the telescope side, the PCU side).

So that's a similar story from the spaceflight archives, about performing something very hard to do but something that to be done.

 

[Max-Q]

Sure, do it with the move to 64 bit. But why not leave 32 bit OpenOrbiter backward compatible?

 

[DaveS]

Sure, do it with the move to 64 bit. But why not leave 32 bit OpenOrbiter backward compatible?

I guess an end point could be designated but where it should it be?

 

[n72.75]

I'm not sure that this change actually would break compatibility. First of all.

Second, there has been a loooot of refactoring and code changes since the initial open source commit. Do we know that one of those hasn't broken something?

I think the path forward to a stable "OpenOrbiter-LTS" (32/64) would be making changes like this.

If we get stuck not being able to make changes, we won't be able progress toward anything. But it's also important to remember that we have a complete history of all the commits. We can make a release from any one of them, at any point.


As GLS said. The first thing we should do is figure out of there is a good reason for this or if it was an oversight.

 

[D-mo]

I am planning to keep sizes and layouts of the structures the same. So, that shouldn't break anything. They will just be (a) defined in one place and used everywhere in the project, and (b) written as templates to reduce code duplication.

However, consolidating all the types will require changing the names of some of them. Here we can go 2 ways:

1. We can follow the names used in the OrbiterSDK. Eg, FVECTOR3 for 3-element vector of floats. The Vector and Vector4 classes in the Orbiter will become DVECTOR3 and DVECTOR4 respectively ("D" for double). And so on.

I personally don't like these names (they look like macros), but the advantage of this approach is that this (hopefully) won't require breaking the ABI, which means existing add-ons will keep working.

2. We can pick uniform names such as FVector3, DVector4, DMatrix4x4, etc. IMHO, this would be better in the long term, but it will break the ABI.

What do you guys think?

 

[GLS]

I apologize for the topic deviation, but I just want to remind (again) that several issues were reported in tickets in the previous incarnation of the forum, and AFAIK they still haven't been "published" anywhere. Maybe working those bugs would be more "profitable" to the users and devs than renaming variable types.

Back on topic, IMO keeping the API types is the way to go... but I'm still curious on how this happened.

 

[n72.75]

100% Option 1.

In an ideal world stuff like this could be standardized, but there are some realities of code that's life time has spanned from C++98 to present. There are some other orbiter types that are capitalized (VESSEL, PARTICLESTREAMSPEC, OBJHANDLE, etc), so there is some logic to this convention.

Not breaking existing compiled libraries is great news and we should aim to do that as long as possible without doing anything hacky to get around that fact, or grinding the project to a halt.

Where I will dig my heels in all the way is breaking existing source code. No doing that. please. Orbiter has had extremely good backward compatibility with source code for as long as I remember, and that should stay the same.

 

[D-mo]

I apologize for the topic deviation, but I just want to remind (again) that several issues were reported in tickets in the previous incarnation of the forum, and AFAIK they still haven't been "published" anywhere. Maybe working those bugs would be more "profitable" to the users and devs than renaming variable types.

I'm with you on this and I am willing to help fix some of the bugs. But the problem is, every time I look at the code base, it makes me want to barf. No offense to other devs. Things like this happen with projects over time. That's why we do refactoring.

Once the code-base is cleaned up, it will be easier to locate and fix bugs.

So, here is what I would like to do:

1. Switch to git submodules for dependencies instead of fetching them on every build. This has already been done. See my PR 347 on github. Just waiting for @Xyon or someone else to accept it.

2. Bring the existing code-base into compliance with c++20 standard. For now this mainly consists of adding a bunch of explicit char* casts to make the compiler happy. This has also been done, but depends on the above PR being accepted. You can find it in the cleanup branch of my fork on github.

3. Remove cruft from the Utils folder. This has also been done and is in the prune branch. I've opted for keeping the TileEdit and ShipEdit utils for now. (BTW, ShipEdit has its own copy of the Vecmat.{cpp,h} files with duplicate definitions of the vector and matrix classes.)

4. Clean up and unify some of the basic types, such as vectors and matrices, plus some math constants. If anybody is wondering, here's how many times the Pi constant is defined in the project:

Utils/Shipedit/Vecmat.h:const double Pi    = 3.1415926535897932384626433832795;
OVP/D3D9Client/D3D9Util.h:#define PI 3.141592653589793238462643383279
OVP/D3D9Client/samples/DrawOrbits/Tools.h:#define PI      3.141592653589793238462643383279
Orbitersdk/include/OrbiterAPI.h:const double PI    = 3.14159265358979323846;    ///< pi
Src/Orbiter/Vecmat.h:const double Pi    = 3.14159265358979323846;
Src/Orbiter/D3dmath.h:const FLOAT g_PI       =  3.14159265358979323846f; // Pi
Src/Vessel/Dragonfly/vectors.cpp:const double PI   = 3.14159265358979;
Src/Vessel/Dragonfly/instruments.cpp: {float Pi=3.1415;
Src/Vessel/Dragonfly/instruments.cpp:float Pi=3.1415;
Src/Vessel/Dragonfly/instruments.cpp:float Pi=3.1415;
Src/Celbody/Moon/ELP82.cpp:static const double cpi     = 3.141592653589793;
Src/Celbody/Moon/xephem/astro.h:#define    PI        3.141592653589793

(That's just a quick grep through .h and .cpp files. There might be more.)

5. Make the project cross-platform and decoupled from the MSVC compiler. One should be able to use a compiler of their choice.

TO BE CONTINUED

 

[D-mo]

Just as a note, at some point will need to break the ABI unless we want to stay with char pointers for the rest of our lives...

 

[DaveS]

Just as a note, at some point will need to break the ABI unless we want to stay with char pointers for the rest of our lives...

Yes and that point should be when everything else breaks: with the move to x64. Otherwise we're just introducing headaches where none should exist.

 

[D-mo]

Honestly, we wouldn't have to be worrying about this, if the addon devs would open-source their addons. Or at least, it would be much less of a problem.

But, as they say in France "c'est la vie".

 

[n72.75]

True, but from my perspective it's more about addon devs having to re-write their source to be compatible.

I've very much onboard for dropping char* and moving to std::string for as many things as we can. It's already a security concern, and making sure nothing in an addon overruns its buffer is a lot of additional workload and messy code that addon devs have to contend with. But as DaveS said, let's do it after we have a stable x86 release.

 

[D-mo]

But as DaveS said, let's do it after we have a stable x86 release.

Sounds good. It would be nice to have my PRs accepted at a faster rate though. Otherwise, I will start losing my enthusiasm and go find something else to do.

 

[Xyon]

Yes and that point should be when everything else breaks: with the move to x64. Otherwise we're just introducing headaches where none should exist.

I tend to agree, breaking stuff in this way is inevitable, but we can wrap all the upcoming backwards-incompatible changes into one "big bang" kind of moment, after which users should not expect addons to "just work". It might be worth co-ordinating the planned move with notable addon developers who are still active to try to ensure some of the more popular projects will still be able to function in OpenOrbiter post-change.

Sounds good. It would be nice to have my PRs accepted at a faster rate though. Otherwise, I will start losing my enthusiasm and go find something else to do.

Yes, sorry. Real life is extremely busy for me just at the moment, and the PRs here are quite large, so they take some time to get through. I am looking through them, but slowly, as free time permits.

I apologize for the topic deviation, but I just want to remind (again) that several issues were reported in tickets in the previous incarnation of the forum, and AFAIK they still haven't been "published" anywhere. Maybe working those bugs would be more "profitable" to the users and devs than renaming variable types.

This is also on my list of "things to do", yes, I haven't forgotten about it. My intention is to make them all into GitHub issues - I think all our outstanding stuff is best tracked there, unless anyone disagrees?