Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/differential/965/

Wherever those icons are loaded should be ignored by checkrefs.pl

We shouldn't use such way of dynamic loading until we have atlases for textures. Because currently the AddIcon calling for each sprite means the slow cycle: (init) read data from drive/memory > create a small texture on GPU > fill the texture, (draw) bind the texture > draw the texture. All this makes the RAM and VRAM pretty fragmented.

While that is correct I think it would be reasonably easy to adapt and thus can probably be merged regardless - I'll defer to your opinion but it doesn't seem like we'll use this feature _intensively_.

In D1745#70225, @wraitii wrote:

While that is correct I think it would be reasonably easy to adapt and thus can probably be merged regardless - I'll defer to your opinion but it doesn't seem like we'll use this feature _intensively_.

The network dialog (and possibly the lobby list) should be able to display country flags in the OList (#3787).
But creating an XML file with 250+ times the same icon definition seems not only tedious to create but also tedious to maintain.

The 250 textures isn't the intensive usage? I'm not sure how old video drivers will handle that. It's true that we already have many small textures, but we significantly increase this number here.

If we're commiting this patch now, I want to see some warning about the problem above.

In D1745#70229, @vladislavbelov wrote:

In D1745#70225, @wraitii wrote:

While that is correct I think it would be reasonably easy to adapt and thus can probably be merged regardless - I'll defer to your opinion but it doesn't seem like we'll use this feature _intensively_.

The network dialog (and possibly the lobby list) should be able to display country flags in the OList (#3787).
But creating an XML file with 250+ times the same icon definition seems not only tedious to create but also tedious to maintain.

The 250 textures isn't the intensive usage? I'm not sure how old video drivers will handle that. It's true that we already have many small textures, but we significantly increase this number here.

If we're commiting this patch now, I want to see some warning about the problem above.

That's fair but I guess we could dynamically create the flags and thus only create about 10-20 at any time. Also our GUI is also kind of a mess in that respect - I guess we should prioritise supporting texture atlases. I'm not sure what kind of GPU you have in mind here - we'll use your feedback data to better assess this I suppose.

Regardless I agree that we should probably directly-raise-concern this.

In D1745#70223, @vladislavbelov wrote:

We shouldn't use such way of dynamic loading until we have atlases for textures. Because currently the AddIcon calling for each sprite means the slow cycle: (init) read data from drive/memory > create a small texture on GPU > fill the texture, (draw) bind the texture > draw the texture. All this makes the RAM and VRAM pretty fragmented.

In D1745#70225, @wraitii wrote:

that is correct

Preface:
I get the impression that you two didn't read the patch and spent maybe 2 minutes on your reply.
Now I sat here and wasted hours being infuriated by me having to find the nerves to forensically prove to you that your accusations against the patch are unfounded.
That you are now the majority claiming this patch to be conceptually broken makes it worse, because the majority of expressed opinion is often equated with the truth by observants who didn't read the code either (argumentum ad populum) and the weird individual is easy to be dismissed as a fool (argumentum ad hominem).
Posting that a patch is broken because it does X, even if it actually doesn't do X, puts me into a position where I basically need to contact my lawyer and lay out all the forensic evidence to prove the correctness of the patch,
making me spend a multiple of the time that it took to come up with this patch and verify it for myself, while I could have been productive or could be unproductive in a more happy or satisfying way instead.

If you are sure what you are talking about, please be specific about it.
If you are not sure what you are talking about, express it as a question, hypothesis or conditional statement.

Verification:

1. terms:

When I say GUI page, I don't mean the GUIManager but a GUI page in the stack of GUIManager. (Each icon is loaded upon GUI page init already, not only once upon GUIManager init / gamestart.)
When I speak of Icons, I actually mean the icon type, SGUIIcon, not a sprite, not a sprite call, not a texture. (So AddIcon is not called for each 'sprite' but for each icon. I think you (Vladislav) just equated icon for sprite.)
We need to be precise with the terms we use, otherwise we are open to misconceptions about the code flow.

2. code flow:

2.1. Writing to m_Icons:
As mentioned in the testplan and noticeable when comparing Xeromyces_ReadIcon and AddIcon:
To the TextureManager and Renderer there is no difference between

• loading an XML GUI page definition that includes an XML icon definitions file that creates m_Icons structs of the CGUI page.
• loading an XML GUI page definition that includes a JS file that creates m_Icons of the CGUI page. The only overhead are the JS function calls which are cheap (microseconds) and only occur once upon page load, not creating any fragmentation in the RAM or even VRAM speakable of.

2.2. Reading from m_Icons:
Before and after the patch, the sprites are loaded upon first use!

CGUI::GetIcon is the only place where m_Icons is used.

That function is only called in CGUI::GenerateText and CGUIString::GenerateTextCall.

CGUI::GenerateText is for example called from CButton::SetupText, CList::SetupText, COList::SetupText, CText::SetupText, CTooltip::SetupText.
SetupText is called from CDropDown::HandleMessage and CDropDown::UpdateCachedSize, CChart::UpdateSeries, CList::HandleMessage, CList::AddItem, IGUITextOwner::DrawText, ....

CGUI::GenerateText is the one that calls SetupSpriteCall, which then interacts with the renderer and texture manager.
I could track it further down to CGUISpriteInstance::Draw and the GUIRenderer::Draw, GUIRenderer::UpdateDrawCallCache, but it should already be evident that it's not AddIcon but the gui tag use in texts that determines when the textures will be loaded (if it already wasn't clear from comparing Xeromyces_ReadIcon and AddIcon).

We shouldn't use such way of dynamic loading until we have atlases for textures.

You didn't qualify the "texture atlas", but if you're talking about just caching texture data, that should already be case with m_TextureCache of CTextureManager (other than it being orthogonal to this patch).

There is one of the reasons why I don't want to upload my patches for review or commit anymore. I'm sure you also don't enjoy this either, so why are we even continuing this farce of judging other peoples (or in some instances even your own) patches (committed or not) without reading them.
I only made this an exception here because fpre donated and asked for this patch to be published.

Now should I actually publish this answer that will just get me banned in the long term or delete it, or save it into a textfile in the folder with thousands of patch notes or delete the reply and pretend this never happened like I always did in the last 4 years?
If I don't change my behavior, nothing will change here. I know you didn't intend to put me into that situation where I get infuriated by having to prove the correctness of the patch, but you did put me into the situation where I have to prove the correctness of the patch; and I still have to pay this with my time without getting any benefit or knowledge gain from it. And that was just the first reply, next we will iterate this for N days, never forget about it for the next decade, which if it accumulates can result in animosities.
A reviewer can only show the uploader a missing piece of information if the reviewer has more knowledge about the involved code in advance; or more commonly; if he spends more time than the patch uploader to understand the patch; which can easily consume hours, not two minutes.

In D1745#70200, @Stan wrote:

Wherever those icons are loaded should be ignored by checkrefs.pl

This was a useful remark because I never consider this script, so thanks for pointing it out.
The script is costing infurating amounts of time to fix as well, but that's not a consequence of anyones mistake (negligence of the maintenance of that script if anything, but not a invalid deed of anyone).

Since the purpose of this script is to determine which files are unused,
and since the modus operandi of this script is to duplicate the logic how simulation components and GUI artifacts refer to files,
the most correct solution might be to load GeoLite2 files, see which country codes it contains and then complain about the png files that are not used by GeoLite2.
If there are more users of country flags, these would have to be added to the script as well.

The 250 textures isn't the intensive usage?
It's true that we already have many small textures, but we significantly increase this number here.

Eh, these textures combined are 116 kilobytes and current textures/ content is 6,195 items, totalling 1.5 GB.

Regardless I agree that we should probably directly-raise-concern this.

I wish I could raise a concern on comments.

We shouldn't use such way of dynamic loading until we have atlases for textures.

You didn't qualify the "texture atlas", but if you're talking about just caching texture data, that should already be case with m_TextureCache of CTextureManager (other than it being orthogonal to this patch).

https://en.wikipedia.org/wiki/Texture_atlas
A texture atlas is a convenient way to increase performance by referring to the same texture with different offsets for different meshes. We do not support creating (or using) texture atlases at the moment in the GUI, which prevents us from efficiently drawing these - not that we would since we also don't use instancing to reduce the number of draw calls.

I believe I've read this correctly and Vladislav's concern is that this would allow one to be super stupid and load up a ton of small textures in memory, possibly fragmenting it and reducing performance - which I do believe it _does_ . The again loading it from XML isn't any better in that respect. If Vladislav thought this meant recreating a new texture every time (effectively a memory leak), this is a different thing then.

@elexis I sent you a forum PM, please let me know if you want to use a different place for discussion.

In D1745#70233, @elexis wrote:

I get the impression that you two didn't read the patch and spent maybe 2 minutes on your reply.

I disagree with that.

If you are sure what you are talking about, please be specific about it.

Let's try.

When I say GUI page, I don't mean the GUIManager but a GUI page in the stack of GUIManager. (Each icon is loaded upon GUI page init already, not only once upon GUIManager init / gamestart.)
When I speak of Icons, I actually mean the icon type, SGUIIcon, not a sprite, not a sprite call, not a texture. (So AddIcon is not called for each 'sprite' but for each icon. I think you (Vladislav) just equated icon for sprite.)

I was talking about low-level textures. I called it sprite, because you called the parameter's name so.

The only overhead are the JS function calls which are cheap (microseconds) and only occur once upon page load, not creating any fragmentation in the RAM or even VRAM speakable of.

It depends on driver, because you create a lot of new small textures.

Before and after the patch, the sprites are loaded upon first use!

It's true, we load textures when we need them.

You didn't qualify the "texture atlas", but if you're talking about just caching texture data, that should already be case with m_TextureCache of CTextureManager (other than it being orthogonal to this patch).

Texture atlas is a way to combine multiple small textures in a big single texture, that decreases number of context switching and memory fragmentation.

Eh, these textures combined are 116 kilobytes and current textures/ content is 6,195 items, totalling 1.5 GB.

I wasn't talking about total size of textures, but their numbers and number of context switchings. Also you can't be sure that it doesn't cost more, because a GPU driver can use an additional memory for each texture.

There is a big difference between before the patch and after the patch. Before the patch you should add all textures (icons, sprites, whatever it's called) through XML. It means that the game knows about all textures before the game starts (or at the same time) and can optimize their loading and atlas packing. But after the patch textures can be loaded dynamically and randomly and engine can't predict altas size, its number.

But creating an XML file with 250+ times the same icon definition seems not only tedious to create but also tedious to maintain (imagine wanting to change the icon size for all flags, removing removed flags, adding new flags).

We may use scripts for such type of icons, like we do for translations.

Now some numbers
For "After the patch" I added 247 flags (16x11 PNG) through the iconTag (like we have 247 players from all possible countries). I recorded all information from the low level - GL textures. Everything is tested on low settings.

About texture context switching (it also costs time, even for very small textures):

As you can see, without texture atlas it significantly increases the amount of work for the texture context switching in a worst case and the number of textures in common case. So it can be used for small number of textures, but it's still dangerous.

I wish I could raise a concern on comments.

@elexis I can understand that you don't know some low-level graphics specific stuff and I describe it unclearly. But I disagree with the charge that I didn't read the patch.

Now some numbers
I added 247 flags (16x11 PNG) through the iconTag

So the numbers show us that displaying 250 new textures means that 250 more textures are processed, but that's not surprising, is it?

Displaying 250 more textures that are defined in memory after they have been loaded through XML once upon init of the current GUI page costs exactly as much memory/performance as displaying 250 more textures that are defined in memory after they have been loaded through JS once upon init of the current GUI page.

Therefore if one wants to measure any performance difference of this patch, it should compare the JS vs XML difference, since that is the only thing that this patch changes.

The texture loading mechanism is _exactly_ the same with and without the patch, so I expect that you get exactly the same memory use with or without the patch if you display 250 textures at the same time.

Notice that this patch does actually not propose to display 250 textures simultaneously as you mentioned.
If you want to go for maximum count of sprites and icons, try the structree (70 icons?) and the training panels (50 icons globally?).
For the network dialog use case, it's more likely that it would display 2-20 additional icons, and that's nothing out of the ordinary at all.
Regardless whether all 250 icon definitions are loaded in or not.

In fact with XML one has to load all 250, with JS one can load less if one wishes to.
The icon definitions have no interference with GL at all, but are only some C++ gui page std instances storing variables.

There is a big difference between before the patch and after the patch.
Before the patch you should add all textures (icons, sprites, whatever it's called) through XML. It means that the game knows about all textures before the game starts (or at the same time) and can optimize their loading and atlas packing.
But after the patch textures can be loaded dynamically and randomly and engine can't predict altas size, its number.

But this is still not true with the actually existing codebase, because

1. The XML icon files are loaded not before, and each time when a GUI page is loaded that loads the XML files, not 'before the game starts'. That's the XML files, not the textures.
2. The textures used by the icons are not loaded before they are used the first time. That is even more randomly and dynamically.

So the argument is hypothetical, or conditional to the presence of a texture atlas implementation if anything.
If someone would do the effort to implement a texture atlas, it would simple if not trivial to use it for icons as well, regardless whether that information is specified in JS or XML.

Just like it is now simple to create a huge XML file, that contains all icons, but the performance is exactly the same for a given amount of used icons, and the XML variant is more error prone (imagine deleted or added icons) and harder to maintain (imagine using a new set of icons with different names, or renamed files). Therefore I had to conclude that this patch is the cleaner alternative, with or without atlas.

But I disagree with the charge that I didn't read the patch.

If you read the patch and understood the patch and the stack of the code that this patch calls that is defined in the other files, then we share the observation that it makes literally zero difference to the engine whether icon definitions are passed via XML or JS, but that it only matters how many icons one actually displays on the current page?
I don't understand how these statements that there is a big difference before and after the patch when there actually isn't any difference in the code.

It depends on driver, because you create a lot of new small textures.

The patch doesn't add a single texture, it only allows specifying them in a different syntax, not more not less.
If someone wants to load 10.000 new textures, he can do so regardless of the language.

So if the actual problem is that someone could load many icons, then that is a problem with icon support by itself.
Then the concern should be raised on the commit that introduced icons without texture atlas support, without putting comments or usage limits into the code everywhere to limit icons or sprites, textures, etc.

If someone wants to display arbitrary numbers of icons in a COList, he can do that with or without the patch. But with the patch it will be 5 lines of JS code to read instead of (3+)*|icons| lines of XML, with no performance difference.

Thanks for sharing your knowledge so far, this way we can get to the bottom of this, or something like that.

Edit: If we consider the network dialog use case, we could also have a different icon for every city, there could be tens of thousands of icons, they could be loaded all at the same time, but none of them reach openGL except those 1-20 of the connected clients.

In D1745#72176, @elexis wrote:

So the numbers show us that displaying 250 new textures means that 250 more textures are processed, but that's not surprising, is it?

Of course.

Displaying 250 more textures that are defined in memory after they have been loaded through XML once upon init of the current GUI page costs exactly as much memory/performance as displaying 250 more textures that are defined in memory after they have been loaded through JS once upon init of the current GUI page.
Therefore if one wants to measure any performance difference of this patch, it should compare the JS vs XML difference, since that is the only thing that this patch changes.
The texture loading mechanism is _exactly_ the same with and without the patch, so I expect that you get exactly the same memory use with or without the patch if you display 250 textures at the same time.

You're absolutely right. But my first comment was a little different:

We shouldn't use such way of dynamic loading until we have atlases for textures. Because currently the AddIcon calling for each sprite means the slow cycle: (init) read data from drive/memory > create a small texture on GPU > fill the texture, (draw) bind the texture > draw the texture. All this makes the RAM and VRAM pretty fragmented.

The patch allows to load textures without a strict control and a predictable result.

Notice that this patch does actually not propose to display 250 textures simultaneously as you mentioned.

Yeah, I just use the number of countries.

If you want to go for maximum count of sprites and icons, try the structree (70 icons?) and the training panels (50 icons globally?).

As you can see in the table I already tried.

You're right that we can reach the same hardware state (a lot of small textures). I don't argue with that. Let me add more details.

Many small textures is the bad thing for UI. We need a texture atlas. It allows to combine many small textures on the the same GPU texture. What's a difference of the texture atlas processing for XML loaded icons and dynamically loaded icons:

XML icons
The engine has a list of all possible textures. It can preprocess them (read only headers) to get their sizes (caches sizes if needed). Then it allocates enough space and packs rectangles there. Without any full texture reading. XML also allows to combine icons by types. Another thing that we can do is to load all UI stuff at the start, because it's pretty small as you noticed.

Dynamic icons
The engine doesn't know about existing textures (it may save a size to a cache after each loading, but it still doesn't know other existing textures yet). So when the JS tries to load a texture, the engine search for an existing GPU texture with enough space and places it there (because a replacing is slow). It's slower and requires an additional work for each new texture. But we may cache a list of already loaded textures. And we need to know how and when to update it. Especially if we have a mod that changes the original texture (we still don't know which textures are changed).

So it's the atlas, but we don't have it yet, we only plan it to have. What's the common problem with the dynamic loading, that any mod is able to load any existing texture without our prediction, that doesn't allow us to do an easier optimization. It will be a problem (easier though) even when we will have atlases.

So you're asking for AddIcons instead of AddIcon.

Notice that this isn't the first place where textures definitions are not present when the GUI page is loaded:

grep -R portrait binaries/data/mods/public/gui/reference/
binaries/data/mods/public/gui/reference/viewer/viewer.js: entityIcon.sprite = "stretched:session/portraits/" + g_Template.icon;
binaries/data/mods/public/gui/reference/structree/draw.js: "stretched:session/portraits/"+stru.icon;
binaries/data/mods/public/gui/reference/structree/draw.js: Engine.GetGUIObjectByName("trainer["+t+"]_icon").sprite = "stretched:session/portraits/"+trainer.icon;
binaries/data/mods/public/gui/reference/structree/draw.js: prodEle.sprite = "stretched:session/portraits/"+template.icon;
binaries/data/mods/public/gui/reference/structree/draw.js: phaseIcon.sprite = "stretched:session/portraits/" + prodPhaseTemplate.icon;

The portrait folder contains 574 items, totalling 41.3 MB.

So a texture atlas that requires every possible texture to be known in advance would require to load these 41mb of textures and put them into a texture atlas, even though only a fraction of them will be used per frame.

In D1745#72226, @elexis wrote:

So you're asking for AddIcons instead of AddIcon.

Actually no, though AddIcons is better than AddIcon. I think these functions are dangerous currently.

In D1745#72239, @elexis wrote:

Notice that this isn't the first place where textures definitions are not present when the GUI page is loaded:

grep -R portrait binaries/data/mods/public/gui/reference/
binaries/data/mods/public/gui/reference/viewer/viewer.js: entityIcon.sprite = "stretched:session/portraits/" + g_Template.icon;
binaries/data/mods/public/gui/reference/structree/draw.js: "stretched:session/portraits/"+stru.icon;
binaries/data/mods/public/gui/reference/structree/draw.js: Engine.GetGUIObjectByName("trainer["+t+"]_icon").sprite = "stretched:session/portraits/"+trainer.icon;
binaries/data/mods/public/gui/reference/structree/draw.js: prodEle.sprite = "stretched:session/portraits/"+template.icon;
binaries/data/mods/public/gui/reference/structree/draw.js: phaseIcon.sprite = "stretched:session/portraits/" + prodPhaseTemplate.icon;

Yeah, we have such things. And it's not good that they aren't declarated in XML. That's why it's harder for us to find unused or wrong named textures.

The portrait folder contains 574 items, totalling 41.3 MB.
So a texture atlas that requires every possible texture to be known in advance would require to load these 41mb of textures and put them into a texture atlas, even though only a fraction of them will be used per frame.

We don't need to create an atlas for each possible texture. We just need to know what textures do we have. E.g. we may have an option in XML, like use_atlas="true". And its texture will be loaded and packed automatically.

It's not good that they aren't declarated in XML. That's why it's harder for us to find unused or wrong named textures.

Well, the only thing that this unused references script can find are unused reference, not unused images. It means if someone removes the use of a sprite, the script doesn't complain about it and 2 things instead of 1 unused.
For example there were many styles and theme parts from the previous UI design and even the one before that (from 2003 or something), one can find the commits in the commit history.

Notice that the engine also supports directory includes of JS scripts and XML GUI object definitions. If file includes are a requirement and directory includes prohibited because we want to maintain a checkrefs script, then those could be argued to be changed as well. But I really think we gain more from directory includes, in any of these cases. It means there is actually less hardcoding to get wrong, less hardcoding to maintain, less merge conflicts with different patches, insertion and removal possible for mods without copying the entire file and all of it's hardcodings.

AddIcons

Unfortunate that the texture atlas design is now a precondition to this 10 line patch, but I guess that's necessary.
As long as one JS call does not introduce more overhead than one XML call, the overhead would be the same. (I guess that's by definition true). Yet it means that there is no performance difference between JS and XML, only that JS allows one improper use that XML doesn't.
At least I find only one, that is reloading the same textures multiple times. But that can be avoided by skipping if the texture exists in the engine if one wanted to. The other alternative is letting developers receive the bugs that they program. Adding a library function isn't a bad thing merely because someone can use it improperly.

Loading after the XML init stage allows loading only those textures that are actually used. For example only the portraits for the selected civ, not the textures of all 13 civs.
If one loads a texture atlas into memory for all textures on init instead of on demand, it will waste a inacceptably much memory (a factor of 13 for the structree), no?

So the texture atlas must be structured in a way that resembles it's demand.
For example all techs of one civ into one texture atlas.

Notice that 3D actors have their textures already present in a texture atlas image.

So wouldn't it be the most logical thing to write an XML definition like:

<TextureAtlas>
  <file1>..
   <file2>..
</TextureAtlas>

And then you can do the same without writing every single file:

<TextureAtlas>
  <directory>...
</TextureAtlas>

or in JS:

// This creates one or more texture atlas unless they exist already
Engine.AddIcons("icons/countries/", 16, 11);

I remember having looked into updating checkrefs thing, but there are so many things broken in that file, that this will take several days to get right.
That still wouldn't justify committing the big XML file if we honestly think avoiding it is better, which I still think considering a possible texture atlas implementation for sprites and icons.
But,
specifying directories for icons on a per-directory basis in XML would also address the issue of the hardcoding antipattern, and it would leave a cleaner implementation of the checkrefs script.

So in setup_resources.xml instead of:

<setup>
	<icon name="icon_food"
		sprite="stretched:session/icons/resources/food_small.png"
		size="16 16"
	/>
	<icon name="icon_metal"
		sprite="stretched:session/icons/resources/metal_small.png"
		size="16 16"
	/>
	<icon name="icon_population"
		sprite="stretched:session/icons/resources/population_small.png"
		size="16 16"
	/>
	<icon name="icon_stone"
		sprite="stretched:session/icons/resources/stone_small.png"
		size="16 16"
	/>
	<icon name="icon_wood"
		sprite="stretched:session/icons/resources/wood_small.png"
		size="16 16"
	/>
	<icon name="icon_time"
		sprite="stretched:session/icons/resources/time_small.png"
		size="16 16"
	/>
	<icon name="icon_xp"
		sprite="stretched:session/icons/promote.png"
		size="16 16"
	/>
</setup>

one could have

<setup>
	<icons
		directory="session/icons/resources/"
  		prefix="icon_"
		size="16 16"
	/>
	<icon name="icon_xp"
		sprite="stretched:session/icons/promote.png"
		size="16 16"
	/>
</setup>

Notice that this XML directory include would allow launching two mods that change the resources via adding JSON files, whereas currently they are advised to overwrite this file (and in the best case would add a new XML file for the new resource), when they wouldn't have to change the icon definitions if they add a new icon in the resources JSON file. The script can still check that the pngs are used that exist and that all expected resource pngs exist. (Anyhow, euqfwlgffycekhiakhbakczlypjqotpqraxfxwumdkvizosgol)

For the resource icons, it sounds more sane if the png, both the _small.png used for the icon and the 64*64 .png used for tribute / trade etc. would be specified in the resources.json file than the path being hardcoded.
That would require not including an entire directory of icons generically but inserting it via JS as in the approach of the first revision of this patch.

While teaching the python script to read resources json files might work, teaching it to interpret JS (in case an entire directory is included) sounds like solving the halting problem.
Then again that one folder that was proposed to be included entirely can just be marked as to be ignored by the python script.

On the other hand, it doesn't seem outlandish to assume that resource icons might have different sizes (to be displayed) per resource.
Then it woud be false to specify a png in resource json but not the size; and if the size was specified there, why isnt it specified in an icon XML.

So the problem that setup_resources.xml has to be overwritten by mods that change resources can be combated by making one XML file per resource.
Then mods can mark individual resources as deleted, overwrite them, insert new icons without overwriting existing ones.
Hence setup_resources.xml should be split into five files by the looks of things :-/
Created D2329 for that.

Notice that (as mentioned before) the JS code already does load sprites via JS at runtime in several places and that these places would also have to be changed to XML being used by JS only to have python script updated!?!
The texture atlas can still be built from JS with an AddIcons command, and with an XML support too.
Still seems slightly cleaner and more in line with the rest if it includes a directory in XML than in JS.