search

minetest / minetest

Luanti (formerly Minetest) is an open source voxel game-creation platform with easy modding and game creation
https://www.minetest.net/
Other
10.76k stars 2.03k forks source link

Re-add support for the TGA image file format #11613

Closed erlehmann closed 3 years ago

erlehmann commented 3 years ago
Minetest version

Commit 1e2b6388818fec0d4cdc52f796850bfb7ec3a22e

OS / Hardware

Irrelevant

GPU model: Irrelevant OpenGL version: Irrelevant

Summary

With PR https://github.com/minetest/irrlicht/pull/48 support for many file formats was removed without @hecktest checking if they are actually used. A lot of what @hecktest removed has later been verified as never used, but some of it broke existing usage of Minetest. I doubt anyone ever reviewed that giant code removal commit at a source level, since it removes thousands of lines and was approved in a short time.

In particular, not supporting TGA breaks all existing maps in MineClone2 and MineClone5. Maps are an item that show a TGA image of the state of the terrain at a specific point in time, thus they can not be regenerated.

Criticism by MineClone2 and MineClone5 developers on https://github.com/minetest/irrlicht/pull/48 that they rely on TGA support was initially brushed aside due to lack of understanding by Minetest engine developers about how removing TGA support affects existing worlds and new clients connecting to old servers (MineClone* developers did not explain that in detail). When I explained the extent of the incompatibilities on IRC, it was recognized as a serious issue and it was decided to add TGA support back to the Minetest engine: https://irc.minetest.net/minetest-dev/2021-09-09#i_5872574

This issue serves as a tracking issue and to highlight the problem of removing formats. There is little to discuss if it should be done. In particular, BMP and PNG are actually harder to write and parse than TGA – and changing the implementation of MineClone2 and MineClone5 to write other file formats would not prevent breaking existing worlds.

Steps to reproduce
  1. With a Minetest client compiled from commit 1e2b6388818fec0d4cdc52f796850bfb7ec3a22e with irrlichtmt at commit https://github.com/minetest/irrlicht/commit/4ab3de3bab13c18bc0eed6bac565be3b80ebac10 and MineClone5 0.71.5 (or any later version), acquire an mcl_maps:empty_map item and right click. Notice that no map is displayed and an error is displayed that no texture could be loaded.
  2. With a Minetest client compiled from commit 1e2b6388818fec0d4cdc52f796850bfb7ec3a22e with irrlichtmt at commit https://github.com/minetest/irrlicht/commit/4ab3de3bab13c18bc0eed6bac565be3b80ebac10 and MineClone5 0.71.5 (or any later version), join a Minetest 5.4 server that contains mcl_maps:filled_map items. Notice that no map is displayed and an error is displayed that no texture could be loaded.
  3. Verify that both of the above cases work with Minetest 5.4 and MineClone5 0.71.5 (or any later version) and have been for quite some time.
Relevant PRs

This issue can be closed once it has been verified that those two PRs are merged and add TGA support back in.

Additional Comments

I found about the same amount of TGA textures in my local cache directory as I found skybox JPEG images after about half a year of playing on a small amount of servers.

To anyone pointing out that MineClone2 0.71 contains no working maps, I want to point out that while that release may be on contentdb, it has numerous lag issues due to enchantment meta size. Those have been fixed in later versions of MineClone2, Mineclonia, and MineClone5. Therefore, a number of servers who ran MineClone2 0.71 in the past have not run the version from contentDB for many months; not all of them are public servers.

In general, I think it is a bad idea to remove working features from Minetest without collecting & understanding user feedback, providing deprecation notices for at least one major release, and some kind of upgrade plan.

hecktest commented 3 years ago

I thought Mineclone's map preview case is easily (and more efficiently) covered by #11485? (and even better, if we're talking about something like Minecraft's handheld treasure maps, it sounds like you want #11498)

Know that TGA was never officially supported just like the myriad of other formats irrlicht accidentally happened to accept, lua_api.txt only mentions PNG and the engine assumes PNG files by filling in the missing .png extension where needed. The only argument in favor is that MT's code also internally enumerated TGA as a media asset - but whoever wrote that probably wasn't thinking of the consequences.

TGA isn't very efficient even with RLE and you're probably doing yourself a big disservice by using this format in your server or game, even more so by pre-generating all these maps and sending them to every user regardless of whether they'll ever see the map. I'm not sure but can't the users just fish these maps out of the media cache as well? It really looks like something that should be handled by #11498.

erlehmann commented 3 years ago

I thought Mineclone's map preview case is easily (and more efficiently) covered by #11485?

Not at all, because TGA-encoded maps existed before Jul 29 2021, when that PR was merged.

No released version of Minetest has that new API – mods can not stay compatible with Minetest 5.3 & 5.4 when using it.

Know that TGA was never officially supported just like the myriad of other formats irrlicht accidentally happened to accept, lua_api.txt only mentions PNG and the engine assumes PNG files by filling in the missing .png extension where needed. The only argument in favor is that MT's code also internally enumerated TGA as a media asset - but whoever wrote that probably wasn't thinking of the consequences.

“Even though the Minetest code has explicitly whitelisted the format it for many years, it was totally not officially supported.” … come on, do you think everyone else is stupid? When stuff is not documented, people look into the source code: The Minetest wiki still lists TGA among the file formats that were hardcoded before you removed a bunch of them: https://wiki.minetest.net/Creating_texture_packs#Creating_Textures

TGA isn't very efficient even with RLE

Yeah and the low-effort PNG code from #11485 is also not very efficient at compression. I do not care much about that.

However, I do care about not randomly breaking user worlds. You seem to not care at all about breakage you cause.

and you're probably doing yourself a big disservice by using this format in your server or game, even more so by pre-generating all these maps and sending them to every user regardless of whether they'll ever see the map. I'm not sure but can't the users just fish these maps out of the media cache as well? It really looks like something that should be handled by #11498.

Yes! You are not sure, that is it! You had no idea what the consequences of your actions were.

Please try to be a lot more careful next time you have the urge to remove something you personally dislike.

Preferably: Provide a solution before you cause problems and make sure that you do not cause problem that way.

Regardless of how talented you may be: A person who ruins some kids' game for selfish reasons is usually seen as the villain.

erlehmann commented 3 years ago

@hecktest also stop making stuff up. If you do not know, say it like that. Others can figure whatever it is out for you if you do.

We both now you never really tried to figure out what file formats are used; @appgurueu did that after your PR was merged.

We both also know that the number of computers that can run Minetest but do not have any usable OpenGL 2.0+ support is not the empty set.

erlehmann commented 3 years ago

I'm not sure but can't the users just fish these maps out of the media cache as well?

I have no idea what this has to do with the issue at hand or what you even mean here.

erlehmann commented 3 years ago

I managed to get Minetest into some weird state where due to dynamic_add_media the server crashes when TGA is not supported.

I can't really figure out what I did to get there though. Any ideas?

rubenwardy commented 3 years ago

This has been discussed to death, best to just readd it and do something more productive

sfan5 commented 3 years ago

Please try to be a lot more careful next time you have the urge to remove something you personally dislike.

Preferably: Provide a solution before you cause problems and make sure that you do not cause problem that way.

The same could be said about the removal of .psd, yet we won't be readding that. It all hinges on whether the format is actually in use and that was assumed not to be the case. Ironically the results by appguru do not show TGA in use either, so even if we had done that prior TGA support would have been removed too.

Regardless of how talented you may be: A person who ruins some kids' game for selfish reasons is usually seen as the villain.

"Think of the children" is a talking point used by politicians to push whatever terrible laws they can come up with. I don't think you really want to lower yourself to this level.

erlehmann commented 3 years ago

It all hinges on whether the format is actually in use and that was assumed not to be the case. Ironically the results by appguru do not show TGA in use either, so even if we had done that prior TGA support would have been removed too.

In my opinion, the important question is not “How did this go wrong?”, but “Why could anyone ever think this would go right?”.

If the research would have been done before and there had been a ticket and people would have had a discussion etc. and the format had been properly deprecated, people would have had a chance to speak up before anything happened. @hecktest and others plan to change a lot of things and surely with noble goals – and I want that process to work better in the future, it is better for everyone involved.

"Think of the children" is a talking point used by politicians to push whatever terrible laws they can come up with. I don't think you really want to lower yourself to this level.

I only mentioned kids because most tend to have no idea why stuff is broken and are usually just sad about it when it breaks – but if they are not old enough, you can not expect them to do anything than upgrade, so they rely on proper compatibility. You are right though, it was a bad argument. Breaking stuff affects all users and there are probably a lot of adults who would also have no idea why some stuff in their world disappears and just be sad!

erlehmann commented 3 years ago

The same could be said about the removal of .psd, yet we won't be readding that.

So .psd was removed without any knowledge if it was used too. But did it ever work?

rollerozxa commented 3 years ago

The PSD support would assumedly have worked at the time it was first written for Irrlicht, but PSD files created with modern versions of Photoshop don't work with the PSD loader (I personally tested it). PSD doesn't serve a specific image format niche (which TGA does, because it's very easy to generate programmatically in Lua), and is also a "source" image format similar to XCF for GIMP and PDN for Paint.net in that it's intended to save images non-destructively with whatever modifications have been done (Layers, masks, filters, editable text...) which is not really effective for normal viewing (you would export it into a PNG or JPEG depending on the image), let alone for a game engine. Keep in mind Irrlicht did not implement any of these "special sauce" PSD features from what I can tell, only implementing the most basic featureset effectively turning it into a poorly optimized and proprietary PNG file.