add IQM format support into lib/picomodel

[eukara]

This adds support for the InterQuake Model format that engines such as ioQuake3, FTE QuakeWorld and many more support.
Animations are not supported (just like most other included model formats) so it'll only load the base pose.

[illwieckz]

I assume picomodel is used by q3map2 map compiler, is it used by the level editor as well?

If needed, there is an iqmmodel plugin for the NetRadiant level editor that may be adapted there: https://gitlab.com/xonotic/netradiant/-/tree/master/plugins/iqmmodel
which is based of aaradiant (from Alien Arena project).

[eukara]

To sum up, as it is an addition to picomodel, it'll display IQMs in the editor and compile them into a .bsp with q3map.
Whereas NetRadiant its iqmmodel plugin only display IQMs in the editor (radiant) portion.

[illwieckz]

When I try to load a map from a game with iqm models in .def entities, I get an error:

ERROR: PicoLoadModel: Failed loading model models/buildables/telenode/telenode.iqm
ERROR: PicoLoadModel: Failed loading model models/buildables/reactor/reactor.iqm
ERROR: PicoLoadModel: Failed loading model models/buildables/arm/arm.iqm
ERROR: PicoLoadModel: Failed loading model models/buildables/medistat/medistat.iqm
ERROR: PicoLoadModel: Failed loading model models/buildables/mgturret/mgturret.iqm
ERROR: PicoLoadModel: Failed loading model models/buildables/eggpod/eggpod.iqm
ERROR: PicoLoadModel: Failed loading model models/buildables/overmind/overmind.iqm
ERROR: PicoLoadModel: Failed loading model models/buildables/acid_tube/acid_tube.iqm

And if I attempt to add a iqm model as misc_model I get:

ERROR: PicoLoadModel: Failed loading model /home/illwieckz/.unvanquished/src/res-players_src.dpkdir/models/players/builder/builder.iqm

[eukara]

That error is printed at the top of PicoLoadModel() before my IQM module gets wind of the file.
It most likely means that the model isn't visible to the filesystem Radiant has mounted.
For context, I had to put my test .iqm files into $HOME/.q3a/baseq3/models to be detected by GtkRadiant.

[illwieckz]

Yeah, and this is not the only error I get unrelated to your code. At this point I'm not able to test it on GtkRadiant before maybe a dozen of other non-iqm bugs to fix first.

Anyway I tried the code in NetRadiant tree. I imported the pm_iqm.c file and disabled the iqmmodel code.

On this screenshot, the telenodes (pad with vertical cylinder gfx), reactor (large rotating structure), medistation (pad with flying cross) and armoury (tripod cylinder) and machinegun turrets are entities loaded as iqm models, the alien granger next to turrets is a misc_model and the other granger is a misc_anim_model, everything looks good:

In game, the granger alien model get baked but I see some surfaces issues, maybe that's not a bug but a shortcoming of q3map2's level of details when turning curves into brushes, would you confirm this assumption to be true? is there a way to tweak q3map2 to improve it?

The visual imperfection may be more obvious without shininess, even without knowing the original model, you would notice some weird triangles:

To me, the picmododel code works properly, I'm just unable to test it with GtkRadiant because of other GtkRadiant bugs.

[eukara]

Are you compiling with -meta? Afaik that should attempt to turn everything into trisoup (and not dynamic curves). If it looks better with -meta than your assumptions about the curve conversion may be correct. But it can also be affected on how q3map2 applies lighting to it. I'm not sure if vertexlit/lightmapped surfaces will really keep the look of smoothing groups of a model to begin with honestly.

[illwieckz]

Here is NetRadiant with iqmmodel plugin:

Here is NetRadiant with iqm picomodel:

Here with a nice slider:

https://imgsli.com/NjMxOTc

The scale and texturing seems to be exactly the same, though the draw order in 2D view is reversed between iqmmodel and iqm picomodel, it looks like iqmmodel was at fault: the turrets are expected to be between tyrant (alien model) and camera on bottom left 2D view. Also I assume there should be brushes drawn between iqm models and 2D camera, and iqm picomodel render looks more correct on that point of view.

[illwieckz]

For reference, the code seems to work fine in DarkRadiant as well:

Note: the orientation of the misc_model tyrant is reverted but it is likely a DarkRadiant bug, since iqm models loaded as entities are orientated correctly.

I noticed IQM misc_model works with GtkRadiant, it's model entities that does not work, so I assume the IQM code is now consdidered verified on GtkRadiant by an independant tester. Though, there are a lot of bugs happening right now with GtkRadiant, unrelated to IQM itself.

⚠ Though BEFORE merging, please read the following. ⚠

About GtkRadiant, @eukara said on IRC:

on OpenSUSE I compile with clang because GCC complained about conflicting 'byte' types

On Ubuntu I didn't have any problem with byte definition but well…

On the mean time I wrote a commit to move some include and remove a byte redefine (!) in a way it makes the patch to be unmodified when ported to NetRadiant and DarkRadiant (if we ignore the file paths being different on DarkRadiant).

Look at my GtkRadiant iqm branch: https://github.com/illwieckz/GtkRadiant/commits/iqm

I added a commit named picomodel: move bytebool.h include in picointernal.h to be reusable, if you import it before yours, you can then import the other commit named fixup and fix-up it to your commit.

This is doable on NetRadiant as well: https://gitlab.com/illwieckz/netradiant/-/commits/iqm/

And on DarkRadiant as well: https://github.com/illwieckz/DarkRadiant/commits/iqm

This would clean-up the byte redefine (and maybe silence the error on OpenSUSE), but would also turn the patch to be more easily portable on other radiantd. The possible per-radiant variations would be living on commit about bytebool.h and the add IQM format support into lib/picomodel commit would be the same everywhere.

[illwieckz]

The reason why the entity models were not displayed in GtkRadiant was because it looks like GtkRadiant does not load entity models from mod folder. As soon as I rename the mod folder with the base folder name, entity models load properly:

On that screenshot, the Tyrant (the huge alien beast) is an misc_model (it was already loading properly before), all the other ones like machingun turrets, reactor, telenode (player spawn) etc. are loaded as map entities (loading properly when stored in base folder and not in mod folder).

So everything looks fine on IQM side. Though @eukara I would appreciate if you apply the small patch picomodel: move bytebool.h include in picointernal.h to be reusable from https://github.com/illwieckz/GtkRadiant/commits/iqm before yours, and squash the fixup commit to yours as it would make easier to port the iqm patch to others radiants and also it may fix your compilation issue on GCC (please confirm!).

[eukara]

The Clang comment I made on IRC was unrelated to the picomodel contribution (afterall, I had to compile GtkRadiant at least once before attempting to port my changes), so your above commit will not fix this I'm afraid. The codebase is riddled with errors related to conflicting 'byte' definitions across the C++ parts of the codebase on the latest GCC because std++ already defines it. I already tried the above (and more) but once I had my fingers in half a dozen files I knew someone with a better overview should tackle this if I can just use Clang and forget about it. Merging the fix doesn't 'fix' it for me anyway so I'm not sure we should proceed with that. It's a separate issue.

[illwieckz]

Maybe your Clang/GCC issue was something else, but definitely including bytebool.h in pm_iqm.c looks to be a workaround and there seems to be a cleaner solution. The byte type is already defined in another header of the picomodel library, so better move that definition at the right place instead of relying on two definitions for the same type in the same library.

At first (at the time I wrote my previous comments), I also included bytebool.h to define byte, but it looks like it's better to not include it and just move the definition at the right place, because the picomodel is a library living its own life, it's probably better to not introduce this or that radiant speficity, for example some radiant may not provide bytebool.h at all, and it's useful to make easier to sync the various instances of picomodel.

So I updated my branch to even not rely on bytebool.h: https://github.com/illwieckz/GtkRadiant/commits/iqm I split the commits on purpose to make it more readable, but they can be squashed.

The thing is that if such patch is applied on any radiant tree prior to your patch, no one has to include bytebool.h in pm_iqm.c, neither apply any specific radiant variant. This is just moving the byte definition from pm_fm.h to picointernal.h so pm_iqm.c does not have to redefine byte neither include bytebool.h.

On my end, if you apply this minor change to your patch I would consider it ready to merge, @TTimo would still have the last word for GtkRadiant, but I would probably merge it to NetRadiant right after that.

diff --git a/libs/picomodel/picointernal.h b/libs/picomodel/picointernal.h
index 28e6e0ef..ac6499c6 100644
--- a/libs/picomodel/picointernal.h
+++ b/libs/picomodel/picointernal.h
@@ -77,6 +77,10 @@ extern "C"
 #define PICO_IOERR  2
 
 /* types */
+#ifndef byte
+       typedef unsigned char byte;
+#endif
+
 typedef struct picoParser_s
 {
        const char *buffer;
diff --git a/libs/picomodel/pm_fm.h b/libs/picomodel/pm_fm.h
index 6fa317ca..8ffcc405 100644
--- a/libs/picomodel/pm_fm.h
+++ b/libs/picomodel/pm_fm.h
@@ -76,10 +76,6 @@
 #define INFO_HEIGHT 5
 #define INFO_Y ( SKINPAGE_HEIGHT - INFO_HEIGHT )
 
-#ifndef byte
-       #define byte unsigned char
-#endif
-
 
 //
 //     Generic header on every chunk
diff --git a/libs/picomodel/pm_iqm.c b/libs/picomodel/pm_iqm.c
index 1bb9d1e9..739cbeff 100644
--- a/libs/picomodel/pm_iqm.c
+++ b/libs/picomodel/pm_iqm.c
@@ -34,7 +34,6 @@
 
 /* dependencies */
 #include "picointernal.h"
-#include "bytebool.h"
 
 extern const picoModule_t picoModuleIQM;

Downloadable patch: https://dl.illwieckz.net/b/gtkradiant/bugs/iqm-model/picomodel-change-request.patch

I assume it's better to squash this change with your commit. In the end it seems cleaner to move the byte definition already provided by picomodel in a place that can benefit to pm_iqm.c than including a GtkRadiant-specific header (not a picomodel one) and doing two definitions of the same type in the same library.

[illwieckz]

@TTimo this looks good to me, though I would appreciate the two commits being squashed (there is a GitHub option to do it at merge time).

Note: I don't know what is the GtkRadiant-build-commands.json file for so my review does not apply to it.

[illwieckz]

@TTimo do you need something else to merge it? (see also my comment above about squashing, that would help porting).

[TTimo]

Thanks for reviewing and testing @illwieckz

[illwieckz]

I merged the patch to NetRadiant: https://gitlab.com/xonotic/netradiant/-/merge_requests/184

I opened a pull request with the patch on DarkRadiant: codereader/DarkRadiant#17

[illwieckz]

For reference, the patch just got merged in DarkRadiant as well:

• add IQM format support into lib/picomodel codereader/DarkRadiant#17
• https://bugs.thedarkmod.com/view.php?id=5712

IQM everywhere! 🎉