game crashes when loading some TCs with "broken" soundpacks

[KangaRoo1372]

Hey, not sure if this is the right place, but I just wanted to let you know about one issue which I discovered about Liero orbmit edition.
Some Liero soundpacks (.snd files) are missing (mostly intentionally) single sounds, e.g. a soundpack in Liero TC called Tank Wars II by Runestorm is missing DROPSHELL and BURNER sound. However, you can still run such TCs normally (either using 1.36 openliero.exe or without it, e.g. when you run liero.exe in dosbox) and this has no impact on the game - even if you set such "empty" sound as a launch sound for any weapon, the game will not crash.
However, when you convert such TC (with such "incomplete" soundpack) to the 1.38+ orbmit version, the game will crash when you try to open this TC in the game. After conversion, the "missing" sound is in fact represented as empty .wav file in the TC/X/sounds subdirectory, but the game cannot read it and the game crashes (with no "warning message" printed).
PS. big respect to you guys for making Liero still alive & developing it, you are the best!

[TimmermanV]

These 'incomplete' are probably created with SndTools which allows users to disable a sound if it is not needed. I wasn't aware the TC tool didn't fully support this. Are you sure the issue is specific to the orbmit fork? I don't think this fork changed the TC tool code much.

[KangaRoo1372]

Yeh, I think you are right about the disabled sounds with SndTools, this might be the case.
To be honest, I am not sure if the issue is specific only to the orbmit fork, actually I didn't check it 🙈 sorry.

[ahockersten]

@KangaRoo1372 could you link or upload a TC that fails, and I'll try to look into this when I have some time left over (which, to be honest, could be a long time - no promises)?

If anyone else wants to look into it and create a patch here or for @gliptic/liero that would of course be very welcome.

[KangaRoo1372]

@ahockersten sure, it's there: https://liero.nl/download/461/tankwars2.zip
no need to rush ofc with fixing that issue ;) thx in advance!
btw, not sure if that matters but I tested it on windows version only (I didn't check how it works on linux).

[ahockersten]

I think I have fixed this now, see 018647c

[TimmermanV]

I wonder if leaving the item out (i.e. not adding it to the array) will cause issues. Are you sure all sounds are played correctly with this fix? Aren't sounds played according to their index in the common.sounds array? E.g. soundPlayer->play(22); and weapon fire sounds etc.

[ahockersten]

Reopening this for additional verification, because I'm not sure how to properly verify this with regards to @TimmermanV's comment.

@TimmermanV thanks for your critical input! I did consider whether this change would have the effect you describe when I made it. I did briefly test with the Tank Wars TC and didn't notice anything that seemed wrong (although Liero was apparently playing the wrong death sound for multiple years without anyone noticing so I'm not sure how big of an indicator that is).

Another thing i considered was how this patch would affect regular Liero, and it should be unaffected, since it does have all the sound samples. This change only ought to affect TCs that were not working before. So in the end, it seems like a very "safe" fix (even if it turns out to be incorrect).

[TimmermanV]

I created a snd file with just the 'BEGIN' sound (index 22) disabled. bugtest.zip Try converting that with the tc tool. If everything works correctly, Liero should play no sound when starting the game. However, I think it may play the ´DROPSHEL´ sound instead, which will indicate that the order of sounds matter in the sounds array.

To solve it, I see two options:

1. Do a two step lookup when playing sounds. E.g. play(22) should be translated to play("BEGIN") by doing a lookup in the original list of sound names, and then the corresponding sound should be found in common.sounds.
2. We ensure common.sounds always has the same amount of entries in the original order and allow for empty/dummy entries.

I think the first option might fit best with the current code and would require fewer changes. Of course, it is slower, and generally I would avoid string comparisons, but it's not like there are a million sounds so I don't expect any noticeable delay.

[ahockersten]

I've tested this now and sure enough you are right, the wrong sound is played with that sound file.

[ahockersten]

As for how to solve this, I'd prefer to do (1) because I think the Liero source code in general could gain from being less obtuse, and we should have the CPU cycles to spare.😄

[TimmermanV]

Allright, I can do that. Would it be best to fork gliptic/liero to apply this fix, or should I fork this repository instead?

[TimmermanV]

I made a quick fix for this for gliptic/liero: gliptic#25
Apparently it cannot be merged directly here, I guess due to the SDL2 changes?

[KangaRoo1372]

I didn't expect this thread to spark such a discussion and be so extensive ;) thank you guys for all the effort you took to fix this (all in all) minor issue - you both are the best.
The fix with using string comparison is pretty nice, however I see some potential issues with that. I'm not sure how it works in 1.37+, but I made some tests and seems like Liero 1.36 (openliero.exe) can handle .snd files with more than 30 sounds, and even with different sound names (different than the original ones). So I guess that if we use string comparisons, there will probably be problems with such "expanded" soundpacks.
[edit: I checked this in Liero 1.38 orbmit edition and it works there too - such soundpacks are converted properly with the TC tool and the game works normally with such sounds.]
However, since actually literally all old classic soundpacks use no more than 30 sounds and old sound names (and afaik there were no tools back in the days with which you could create such "extended" soundpack), then I guess this is a minor issue. ;)

[TimmermanV]

Well, in that case we may still need a vector of at least 30 entries, some of which can be dummies/disabled. Hmm, I think you could make a TC that breaks hard coded sounds though. Maybe a better solution would be to change the way the code (menu / weapons / special objects etc) references sounds, so they always use strings instead of integers.

[TimmermanV]

Do you have any example of such extended SND files? I'm not sure if we'd need to support that. Supporting more than 30 sounds in the openliero TC format however seems reasonable and sensible.

[KangaRoo1372]

@TimmermanV yeh, you can find such SND file on my gitlab here: https://gitlab.com/KangaRoo1372/test/-/blob/main/utest.zip (the zipped SND file is named "sounds" because it was created for webliero, so of course remember to rename it to "Liero").

[TimmermanV]

I now think the nicest way to fix this is to get rid of integers to refer to sounds altogether. Instead, sounds should be referred to by their name. To change this would mean breaking compatibility with converted TCs as the format of the configuration files will change.

If that is not a desired direction, than this might be an alternative:

• use the order of the sounds from tc.cfg (instead of the original list of sounds)
• fix the exe reader / tc tool so it remaps sound indices in case any of the sounds are disabled
• change hard coded sounds to use sound names (so no play(22);)

[ahockersten]

@TimmermanV personally I'm OK with breaking the format of TC configuration files as you suggested, since they can be regenerated and old compatible versions of Liero always remain available.

Zooming out a bit, when it comes to "desired directions" in general for the Liero codebase, I don't personally have any strong ones. I'm surprised that it still builds and runs and any changes that keeps it building or makes it easier to hack on are good in my book. Actually I might even go so far as to say that I'm happy if I see anyone who wants to work on improving/changing/experimenting with Liero, even if the changes end up being "bad". It's still more work than I've done on it in the past 5 years or so. 🙂

[TimmermanV]

Great, I think I'll give that a go then.

That leaves my other question, should I in general fork gliptic/liero, or this repository? The orbmit fork seems to be the main active one, though depending on the change both could benefit from / merge changes. What is your take on this?

[ahockersten]

Well, originally when I made my fork gliptic wasn't very active so I ended on diverging quite a bit. My features and patches ended up a bit messy and tangled as a result.

I think it might be time to fix that, it would make maintaining my fork much easier (if, at the end of the day, there's nothing left to maintain I wouldn't mind 😄). So suggestion is to do it on gliptic/liero, and I will open a separate issue there to figure out a plan to get SDL2 and whatever else is useful on there.

Edit: separate issue on gliptic/liero gliptic#26