Dropping Legacy Support

[aeonios]

It's come to my attention that there was an approved plan for dropping FFP and ARB shader support some 6 years ago, but the dev that was working on it left so it never got done, and we've been arguing about it on and off ever since. According to a survey taken some 3 years ago or so over 95% of our users have support for at least openGL 2.1 (shader version 120) and more than 45% have support for  openGL 3.3 or newer, while a good chunk support 3.0 or 3.1 as well.

So this is what I propose:

- We remove all traces that FFP ever existed, from config, shader effects, and anything in the codebase relating to those things.

- We remove all traces that ARB shaders ever existed. There is basically no one using them and no one working on them.

The legacy support is cluttering up the codebase and making it more difficult to support newer and more useful things.

Additionally, I propose we add support for:

- Automatically detecting hardware GL version to support whatever 3.0/3.1/3.3/4.x features the user's hardware allows (and that we can find good use for).

- Allowing for choosing different GLSL shader versions automatically based on GL version support.

- Creating a config/command line option for spoofing older GL versions for testing purposes, so it's possible for devs to manually choose which shader versions will be loaded.

- And of course support for draw-call reducing features from GL3.1-3.3, based on whatever the user's hardware supports. Since some things have lower version requirements than others it makes sense to allow such things to be enabled incrementally so users can get as much as their hardware can provide.

Finally, I think we should establish shader version 120 as the standard for legacy OpenGL 2.x support, since basically no users have GL 2.0 graphics cards and virtually everyone has support for 2.1. GLSL version 120 supports a couple of very convenient features that allow for efficient shaders, at least as far as what 2.x supports in terms of performance. Not every shader needs to be declared as #version 120 (if it doesn't matter then who cares), but it should at least be made allowable without question wherever it's needed.

Note that a lot of shaders will likely remain as GL2.x simply because there's no benefit to upgrading them. The water shader and postproc shader(s) and even the terrain shader are good examples. At the moment the only shaders that I'm aware of that would benefit from GL3.x are model_common and the particle shader, to support drawinstanced and soft particles.

[vladislavbelov]

I definitely agree with that. But currently engine isn't ready to easily introduce major version dependencies. We'll get a spaghetti code or even performance regressions. So it needs to do it carefully.

 

[aeonios]

4 minutes ago, vladislavbelov said:

I definitely agree with that. But currently engine isn't ready to easily introduce major version dependencies. We'll get a spaghetti code or even performance regressions. So it needs to do it carefully.

 

No, spaghetti code is what we have right now. We have a mess of "shader techniques", "materials", and all kinds of hacks for supporting fossilware. If anything it should be a lot cleaner to only have to worry about GLSL versions and draw call techniques (although we may need to modularize the draw call techniques since they may require very different things, but that shouldn't be too difficult). I mean the current renderer uses structs inside of classes for some unfathomable reason (object instances are structs!) and keeps options settings in 50 different places with all kinds of backwards interdependencies (like the options-parser needing to call the postproc manager directly). It's an unholy mess and it should probably be cleaned up along the way.

[vladislavbelov]

12 minutes ago, aeonios said:

No, spaghetti code is what we have right now.

Ehm? I didn't say anything about the current state.

14 minutes ago, aeonios said:

draw call techniques (although we may need to modularize the draw call techniques since they may require very different things, but that shouldn't be too difficult).

Hehe, instancing may change batching very well. But I don't want to say many words, it'd be good to analyze solutions when they will be ready.

15 minutes ago, aeonios said:

I mean the current renderer uses structs inside of classes for some unfathomable reason (object instances are structs!)

What's the bad? Do you know difference between struct and class?

16 minutes ago, aeonios said:

keeps options settings in 50 different places with all kinds of backwards interdependencies (like the options-parser needing to call the postproc manager directly). It's an unholy mess and it should probably be cleaned up along the way.

True, it should be solved.

[aeonios]

25 minutes ago, vladislavbelov said:

What's the bad? Do you know difference between struct and class?

Yes, a struct doesn't use inheritance, which makes function calls direct and can improve performance if the compiler doesn't support class hierarchy analysis (which you can do the same thing by declaring a function as final). Otherwise there's no difference. The bad is that it uglies up the code a lot by creating a whole sub-class within an existing class file, and then you have to make function calls that explicitly reference the struct rather than just calling the function normally as a local function or class method. The class which should normally be self-managing now has to manage another "class" inside itself for no good reason.

[vladislavbelov]

 

4 hours ago, aeonios said:

Yes, a struct doesn't use inheritance, which makes function calls direct and can improve performance

Are you sure, that you're talking about C++? (It seems true for C#, no?) struct and class are the same except only one case: default members/methods and base access. Inheritance it's the same. Struct are used as PODs, but they can have inheritance, virtual methods, explicit ctors and other "class things". Compiler optimizes classes and structs equally. If there're virtual methods (vtable costs, but last time not so big as earlier) and/or inheritances, it'd be a harder work for compiler for both.

4 hours ago, aeonios said:

The bad is that it uglies up the code a lot by creating a whole sub-class within an existing class file, and then you have to make function calls that explicitly reference the struct rather than just calling the function normally as a local function or class method. The class which should normally be self-managing now has to manage another "class" inside itself for no good reason.

I'm not sure, that I'm understanding you correctly. Structs are used as PODs. Subtypes are usually needed to have a shorter name, do not create an additional useless class file for a small struct and logically splited types.

Compare:

class TextureManager
{
public:
      struct InitParams
      {
      	...
      };

      TextureManager(const InitParams&);
};

And yours:

struct TextureManagerInitParams
{
  ...
};

class TextureManager
{
public:
      TextureManager(const TextureManagerInitParams&);
};

For me it's useless to make them globally visible.

[aeonios]

10 hours ago, vladislavbelov said:

 

Are you sure, that you're talking about C++? (It seems true for C#, no?) struct and class are the same except only one case: default members/methods and base access. Inheritance it's the same. Struct are used as PODs, but they can have inheritance, virtual methods, explicit ctors and other "class things". Compiler optimizes classes and structs equally. If there're virtual methods (vtable costs, but last time not so big as earlier) and/or inheritances, it'd be a harder work for compiler for both.

I'm not sure, that I'm understanding you correctly. Structs are used as PODs. Subtypes are usually needed to have a shorter name, do not create an additional useless class file for a small struct and logically splited types.

Compare:

class TextureManager
{
public:
      struct InitParams
      {
      	...
      };

      TextureManager(const InitParams&);
};

And yours:

struct TextureManagerInitParams
{
  ...
};

class TextureManager
{
public:
      TextureManager(const TextureManagerInitParams&);
};

For me it's useless to make them globally visible.

Eh, after looking it up it seems like the only difference between a C++ class and a struct is that a struct's members are all public by default. The real issue is that dumping everything into a struct turns the renderer into a render-options-manager rather than just a renderer. Things like init params are usually based on some external data, which should just be encapsulated in its own class, which is then passed as an argument in the constructor or initializer. The current way things are constructed and initialized does not make sense, and there are lots of classes that should really have an init() method and don't.

In other words what I would do is something more like this:

class TextureManager
{
public:
	TextureManager(CUserOptions userOptions);
		// it reads what it needs and initializes its own private variables with no struct
}

In other words, using PODs in an OO language is bad form.

[vladislavbelov]

 

3 hours ago, aeonios said:

Things like init params are usually based on some external data, which should just be encapsulated in its own class, which is then passed as an argument in the constructor or initializer. The current way things are constructed and initialized does not make sense, and there are lots of classes that should really have an init() method and don't.

Vertex structs don't have such data and they don't need to have init().

3 hours ago, aeonios said:

In other words, using PODs in an OO language is bad form.

I can't agree with that, for me it sounds like the + operator is bad in an OO language and you should use add() method instead.

PODs are used for simple data without special ctors, and you don't need to add and call ctor and/or many setters to initialise the data. I.e. PODs are easy for using as vertex attributes.

[aeonios]

50 minutes ago, vladislavbelov said:

Vertex structs don't have such data and they don't need to have init().

Vertex structs are a special case. One that has nothing to do with using a struct for the settings and data of every class.

52 minutes ago, vladislavbelov said:

I can't agree with that, for me it sounds like the + operator is bad in an OO language and you should use add() method instead.

PODs are used for simple data without special ctors, and you don't need to add and call ctor and/or many setters to initialise the data. I.e. PODs are easy for using as vertex attributes.

I'm not suggesting that we use setters and getters for everything. Just that we should maintain a proper separation of concerns and avoid making structs for every class's data, and avoid keeping options spread all over the place or creating a mess of interdependencies in the process. Why keep something in a struct when it could work just as well as an instance variable?

[vladislavbelov]

On 5/6/2018 at 2:39 PM, aeonios said:

Vertex structs are a special case. One that has nothing to do with using a struct for the settings and data of every class.

Ok, let's see inplace.

On 5/6/2018 at 2:39 PM, aeonios said:

avoid keeping options spread all over the place or creating a mess of interdependencies in the process

I agree to avoid such things.

On 5/6/2018 at 2:39 PM, aeonios said:

Why keep something in a struct when it could work just as well as an instance variable?

As I said, structs are mostly used as PODs, but it has different usings, that allow you to write less code and less make errors.

Constructors and methods calls

Sometimes you need to pass many params to constructors or methods, and pass them directly may lead to errors and a unreadable code.

Without struct:

class TextureManager
{
public:
	TextureManager(size_t cacheSize, bool preallocatedCache, bool generateMipmaps, ...);
};

// It's pretty easy to forget and swap bools here.
// And the call is pretty big. You need to add comment to everyone,
// but it will break after an order change in the constructor without
// compilation errors.
... = std::make_shared<TextureManager>(10 * MiB, true, false, ...);

With struct:

class TextureManager
{
public:
  	struct InitParams
	{
		size_t cacheSize;
		bool preallocatedCache;
		bool generateMipmaps;
		... 
	};

	TextureManager(const InitParams&);
};

// It's easy to catch missed params.
TextureManager::InitParams params;
params.cacheSize = 10 * MiB;
params.preallocatedCache = true;
params.generateMipmaps = false;
...
... = std::make_shared<TextureManager>(params);

And much bigger production example: https://cs.chromium.org/chromium/src/ui/views/widget/widget.h?sq=package:chromium&l=149.

Function results and grouped members

Sometimes function should return more than value, popular way is add ref/pointer params (it's pretty fast, but more symbols and the same problem with number of params). But with struct it can be solved shorter. I.e. std::pair is struct.

Without structs you need to write a getter for each property, and sometimes these properties don't need to be access and store separately. Example: our Material class. With struct it allows to use only one std::vector to contain params. And there is no needs to use classes, because the data is pretty simple.

 

 

 

[aeonios]

A bit of an update since I've had the opportunity to work on my own mod I've gotten a much better picture of engine internals I now have a better idea of how things work and where draw calls are coming from.

1. Unfortunately we cannot drop legacy support unless we can successfully track down the glsl crash that some people are still experiencing, and thus far no luck getting someone with technical knowledge to reproduce it.

2. The reason why a lot of units and buildings take up a lot of draw calls is because they use a lot of props as sub-objects. The props use a separate model and texture from the main object which means that multidrawelements would not do us any good. However for props which are rigid (which is most of them, except for heads maybe, although I haven't paid that much attention to the animations) we could most likely use drawinstanced instead, which would amortize away most of the cost of drawing units/buildings when there are a lot of them. The good news is that those things probably consume the majority of all draw calls, so we could get the total number per frame down to a very small number.