Submitting patches

[Funto]

Hi,

I'm occasionally working on the SuperTuxKart project, that uses Irrlicht, and there are some modifications we are making to 1.8 there, that I think would be worth reporting upstream.

I'll start with a simple patch that replaces raw values for the buffer offset in glXXXPointer() functions with usage of the offsetof() macro. I had to do that because I'm currently experiencing with optimizing the skinning part of Irrlicht using SSE (no GPU skinning yet), and I had to modify S3DVertex, etc to comply with SSE constraints. That's where I saw that these raw values were used. I believe using offsetof() is better as we can change what is in the vertex format without having to fix the OpenGL driver.

Here is the patch: https://sourceforge.net/p/irrlicht/patches/274/

[CuteAlien]

Thanks, but one remark: Please don't put stuff in your patches that just makes the lines look prettier for your taste. All that whitespace stuff to make parameters line up in certain columns just makes it harder to see which lines got really changed.

Edit: Btw, first time I heard about offsetof, I've never run into that before (I guess people programming closer to the hardware or in emdedded area will break out loughing hearing that...). Really looks like it's made for just this.

[Funto]

On the remark on formatting, I understand, will try to avoid that in the future.

Can I expect my patch to reach trunk?

[hendu]

The thing is, offsetof is undefined for most C++ classes. It's only defined for C structs (POD data). S3DVertex has a constructor, which means the compiler has permission to kill kittens.

http://stackoverflow.com/questions/3129 ... f-offsetof

Offsetof is undefined for non-POD types. Having a constructor makes it non-POD.

[CuteAlien]

Hm, one of those cases that would work probably with every compiler (as our constructors don't mess with the memory layout I think) but are still undefined :-( Sorry, that means we can't use it.

[hybrid]

But I guess we can adapt the inside mechanisms. After all, this boils down to some pointer arithmetics on top of the vertex structs. IIRC, we have had this discussion some years ago, and I removed the use of offsetof again. But I see the advantages of becoming a little more flexible already now.

[Funto]

It seems that's right, offsetof() is not defined for non-POD types...
However in practice, as long as there are no virtual functions or multiple inheritance, the layout will be standard, and offsetof() will certainly work.
For the record, Visual Studio defines it this way:

Code: Select all

 
#ifdef  _WIN64
#define offsetof(s,m)   (size_t)( (ptrdiff_t)&reinterpret_cast<const volatile char&>((((s *)0)->m)) )
#else
#define offsetof(s,m)   (size_t)&reinterpret_cast<const volatile char&>((((s *)0)->m))
#endif
 

I don't see how having non-POD types would make this not work (the virtual table pointer would be taken into account...).

So, 2 things:
1) In practice, it works in all cases and is portable.
2) Do you really think it's better to have hard-coded offsets that are supposed to match the layout of a structure that is defined independently from those offsets? That's an implicit dependency in the code (which BTW isn't documented). Furthermore, those hard-coded values are written with some given assumption on the way the compiler will pack the structures, which isn't really better than relying on offsetof() to return a coherent value...

[Funto]

No news on this topic then?

One other thing: in the SuperTuxKart project, we had to set _IRR_MATERIAL_MAX_TEXTURES_ in IrrCompileConfig.h to a bigger value (we switched from 4 to .
Is it possible to report this modification to trunk?

For now we have our own version of Irrlicht, and as time goes we tend to get away from the official version (we have SSE optimizations in progress, we may integrate GPU skinning in the future, one of our branches has some modifications to drawAll(), there is this _IRR_MATERIAL_MAX_TEXTURES_ value, an "animation strength" feature...). Linux distro packagers tend to integrate the official Irrlicht, but we need our modified version or the game crashes, so we made a fork, and try not to differ too much from the original version. That's why I'm trying to report at least some of our modifications here...

[CuteAlien]

offsetof got changed in c++11 and is now allowed to work with standard layout classes. S3DVertex has no virtual functions or statics and all members are public, so it seems to fullfil the criteria as far as I can see. On the other hand we don't use c++11 yet and this will likely cause a warning on gcc for now about undefined behavior. Well, I don't care, so if Hybrid likes it, then it's also fine by me. That's more Hybrids area than mine anyway. If we find a 3rd way to do this which makes it flexible without relying on undefined behavior then that would be even better.

I'm for increasing _IRR_MATERIAL_MAX_TEXTURES_ to 8 because 4 is not enough for so many people by now and the additional memory and speed cost sounds acceptable to me (but I have not profiled it, so maybe worse than I expect?). Better to have stuff work by default and people optimizing have to tweak instead of the other way round. But again not my area. edit: Also maybe we should alllow overriding such a number define by another number define, so it can be controlled via compiler flags as well, which is a little easier to customize (not in your project but in mine... adding another project file allows one still to work with original sources without ever getting into merge conflicts then).

Generally about patches - we are happy when people post them, but won't apply them all. And that Linux distributions make it so hard for application developers to work with modified library code is unfortunate and imho completely against the spirit of free software. But obviously other people, like distribution maintainers/vendors, think otherwise and I guess as long as I don't release my own distribution which rocks the world I won't have much of a leg to stand on when complaining.

[Funto]

Ping? Nothing new here?

[CuteAlien]

Well, no. Except that I got a little more worried about increasing to 8 textures by default after I realized that material comparisons include all texture-layer comparisons. And I think those happen constantly to figure out render-state changes. So speed cost is maybe not so trivial.

Anyway I'm working on 1.8.1 release right now and won't put this on my TODO-list as it'll just stay there forever (already promised some stuff weeks/months ago which is still unfinished). And when Hybrid finds time for this I'm sure he will post here.

Please don't ping stuff already after a few days. We already have not as much time as we like for developing Irrlicht and so it's not fun spending it on explanations why we find no time *sigh*

edit: And please don't take this wrong - we love SuperTuxKart and will try to fix it (the first report, the 8 texture things is more tricky as it got discussed already a few times). Just ... probably not right now, so be a little more patient.

[Funto]

I think I'm in a good position to understand how it feels to lack time, no worries I was just checking after some days if the conversation was ended or not. I didn't expect to get answers anymore, sorry if I didn't wait long enough.

The "offsetof" thing isn't really important to be honest, it's a mere improvement for maintainability, there is no additional feature and no fundamental change. It was more of a way to start communication with Irrlicht developers and evaluate how hard it is to contribute, and whether or not we should go with our fork or not.
The 8 textures thing was more problematic as it was the real reason for us forking Irrlicht in the beginning, and it's a real requirement for us.

On the speed cost, best thing would be to just run a comparison before/after the modification on some test scene...
On material comparisons, maybe you could maintain a "highest texture used" variable per material, and comparison would use it to loop up to the highest texture? Just an idea, but that would be only if that really is a performance problem IMHO...

[CuteAlien]

Yeah, the problem with max-textures and distros which don't accept modified libraries has been discussed before. Highest texture used is hard to add because access to material layers is not restricted (aka users can use 0 and 7 for example) and adding access restrictions would break lots of applications. I still think it's probably a good idea to increase it by default ... but again not really my area (I'm mainly the GUI-programming guy...). Maybe I can add least do some profiling, so we get an idea how much it would cost.

[hendu]

While this has been posted before on the 8 textures topic, some recent cards only offer four (legacy) texcoord slots. So unless the work also includes the separation of texture and texcoord, the textures 5-8 would not work on those cards.

As they are recent and that is old GL functionality, it's not something the users will expect.


edit: here's a source: http://feedback.wildfiregames.com/repor ... TURE_UNITS
Geforce GTX 4xx, 5xx: Fermi and Kepler. Current Radeons still seem to expose more.

[Funto]

Hendu >> do you know which cards those are? (I suppose some Intel chips?) So the philosophy is to only expose something that would work everywhere? That severely restricts the feature set Irrlicht can cover, then, and it wouldn't push the engine forward...