Custom map icons support #788

thicco-catto · 2025-12-12T21:08:45Z

Added two functions to Minimap to allow for custom map icons, similar to those of curses or map items.

Minimap.AddIcon(MinimapIconPosition position, string anm2, string anim, int frame)
Adds a new icon to be rendered in the minimap. Returns the id of the icon added, used for removing the icon later on.
Minimap.RemoveIcon(MinimapIconPosition position, int id)
Removes the given icon.

MinimapIconPosition is a new enum with the following values:

BEFORE_MAP_ICONS: Places the icon before any other.
AFTER_MAP_ICONS: Places the icon after all the map item related icons, but before restock.
BEFORE_CURSE_ICONS: Places the icon before all the curse icons, after restock.
AFTER_CURSE_ICONS: Places the icon after all the others.

Guantol-Lemat · 2025-12-12T22:28:58Z

repentogon/LuaInterfaces/HUD/LuaMinimap.cpp

+	int id;
+};
+
+static std::vector<std::unique_ptr<MinimapIcon>> BeforeMapIcons;


Is there a specific reason as to why std::unique_ptr<MinimapIcon> is being used instead of MinimapIcon directly?

I don't think it's necessary personally

However, if we go with a registration setup for all this, these might end up as std::vector<MinimapIcon*/ANM2*>, so they can refer to the registered one we keep stored.

Guantol-Lemat · 2025-12-12T22:37:07Z

repentogon/LuaInterfaces/HUD/LuaMinimap.cpp

+			found = true;
+
+			TakenIds.erase(TakenIds.find(icons[i]->id));
+			delete icons[i]->sprite;


I would suggest making a destructor for the icons, rather than handling deletion manually.

Having the ANM2 stored directly inside of the struct instead of being a pointer would remove the need for manual memory management as well as a destructor.

Or, if we end up not needing the struct, we can work with an ANM2 by itself.

Guantol-Lemat · 2025-12-12T22:38:10Z

repentogon/LuaInterfaces/HUD/LuaMinimap.cpp

+
+int GetFreeId(std::vector<std::unique_ptr<MinimapIcon>>& icons) {
+	int free_id = 1;
+	bool found = false;


this found variable is never used.

Guantol-Lemat · 2025-12-12T22:46:59Z

repentogon/LuaInterfaces/HUD/LuaMinimap.cpp

+		return;
+	}
+
+	icons.erase(icons.begin() + idx_to_delete);


Minor nitpick, but I would suggest placing this in the loop before the break, as it makes more sense. I would also replace the break with a return;

Guantol-Lemat · 2025-12-12T22:50:29Z

repentogon/LuaInterfaces/HUD/LuaMinimap.cpp

+{
+	Minimap* minimap = g_Game->GetMinimap();
+	int priority = (int)luaL_checkinteger(L, 1);
+	int id = (int)luaL_checkinteger(L, 2);


I would suggest making the id into an userdata (or creating a different kind of system) so that we may properly invalidate it, and avoid misuse from users. Would also allow us to potentially alter the implementation of the id if that were to be necessary.

I personally feel like userdata would be overcomplicating things further, but I agree with the general sentiment regarding the ID generation

Like I suggested in discord I feel like one option would be have a function to Register an icon ONCE, then you can freely add and remove it by the given ID.

That would probably also simplify other parts of this, as IDs never need to be freed up, and will only ever be assigned in sequential order

That may require some changes to other bits, though. For example, if we go with registration, we'd probably store all registered icons in a single structure. In theory it could be as simple as a std::vector<ANM2>, with the "ID" actually just being its index. Though a struct would be fine too if we need it.

Guantol-Lemat · 2025-12-12T22:56:12Z

repentogon/LuaInterfaces/HUD/LuaMinimap.cpp

+static std::vector<std::unique_ptr<MinimapIcon>> AfterMapIcons;
+static std::vector<std::unique_ptr<MinimapIcon>> BeforeCurseIcons;
+static std::vector<std::unique_ptr<MinimapIcon>> AfterCurseIcons;
+static std::map<int, bool> TakenIds;


I would suggest making this a std::set given the usage in the other functions, or potentially simplify the id generation.

Guantol-Lemat · 2025-12-12T22:59:09Z

repentogon/LuaInterfaces/HUD/LuaMinimap.cpp

+	}
+}
+
+int GetFreeId(std::vector<std::unique_ptr<MinimapIcon>>& icons) {


icons is never used.

Guantol-Lemat · 2025-12-12T23:03:18Z

repentogon/LuaInterfaces/HUD/LuaMinimap.cpp

+	TakenIds[free_id] = true;
+
+	return free_id;
+}


Currently the Id generation doesn't have any issues, but I think it can be greatly simplified.

Guantol-Lemat · 2025-12-12T23:05:39Z

repentogon/LuaInterfaces/HUD/LuaMinimap.cpp

+
+#pragma region Helper functions
+void TryAddIcon(MinimapIcon icons[], unsigned int& num_icons, ANM2* sprite, std_string& anim, int frame) {
+	if (num_icons < 7) {


I would suggest making this and other magic numbers into const integers with a clear name.

Guantol-Lemat · 2025-12-12T23:16:17Z

repentogon/LuaInterfaces/HUD/LuaMinimap.cpp

+
+struct MinimapIcon {
+	ANM2* sprite;
+	std_string anim;


Since we have full ownership of sprite, anim and frame can be removed, and instead we set the sprite to the appropriate animation and frame during validation. The render step then becomes a simple render all sprites.

Taking it a step further, if we do keep the MinimapIcon struct, it should just contain an ANM2 object directly, not as a pointer. No need to bother with memory management here.

Right now i set it up as a pointer since the vanilla icons all use the same sprite. Before rendering I'm putting them all in a MinimapIcon array. This simplifies the actual render loop, since I don't need to diferentiate between custom and vanilla icons.

You could pass a pointer to the sprite when inserting them in the render queue.

Since the MinimapIcon struct is separate from the RegisteredMinimapIcon struct, then having the anm2 here as a pointer makes sense since the ANM2 is owned by either the game (for vanilla icons) or by the RegisteredMinimapIcon (for custom ones)

So this seems okay as is rn, it being generic for both vanilla and custom icons makes sense.

ConnorForan · 2025-12-13T01:26:04Z

repentogon/LuaInterfaces/HUD/LuaMinimap.cpp

+{
+	Minimap* minimap = g_Game->GetMinimap();
+	int priority = (int)luaL_checkinteger(L, 1);
+	int id = (int)luaL_checkinteger(L, 2);


I personally feel like userdata would be overcomplicating things further, but I agree with the general sentiment regarding the ID generation

Like I suggested in discord I feel like one option would be have a function to Register an icon ONCE, then you can freely add and remove it by the given ID.

That would probably also simplify other parts of this, as IDs never need to be freed up, and will only ever be assigned in sequential order

That may require some changes to other bits, though. For example, if we go with registration, we'd probably store all registered icons in a single structure. In theory it could be as simple as a std::vector<ANM2>, with the "ID" actually just being its index. Though a struct would be fine too if we need it.

ConnorForan · 2025-12-13T01:33:12Z

repentogon/LuaInterfaces/HUD/LuaMinimap.cpp

+
+struct MinimapIcon {
+	ANM2* sprite;
+	std_string anim;


Taking it a step further, if we do keep the MinimapIcon struct, it should just contain an ANM2 object directly, not as a pointer. No need to bother with memory management here.

ConnorForan · 2025-12-13T01:33:42Z

repentogon/LuaInterfaces/HUD/LuaMinimap.cpp

+	int id;
+};
+
+static std::vector<std::unique_ptr<MinimapIcon>> BeforeMapIcons;


I don't think it's necessary personally

However, if we go with a registration setup for all this, these might end up as std::vector<MinimapIcon*/ANM2*>, so they can refer to the registered one we keep stored.

ConnorForan · 2025-12-13T01:41:21Z

repentogon/LuaInterfaces/HUD/LuaMinimap.cpp

+			found = true;
+
+			TakenIds.erase(TakenIds.find(icons[i]->id));
+			delete icons[i]->sprite;


Having the ANM2 stored directly inside of the struct instead of being a pointer would remove the need for manual memory management as well as a destructor.

Or, if we end up not needing the struct, we can work with an ANM2 by itself.

ConnorForan · 2025-12-13T01:46:19Z

repentogon/LuaInterfaces/HUD/LuaMinimap.cpp

+	int priority = (int)luaL_checkinteger(L, 1);
+	int id = (int)luaL_checkinteger(L, 2);
+
+	if (priority == ICON_POSITION_BEFORE_MAP_ICONS) {


Sorry for asking this question, but how necessary is the priority, really? I understand the purpose of it, but personally I feel like we could also justify just having it so that all custom icons are placed after the vanilla map icons. Would that be so bad?

I'm not saying that you can't have the priority system, but I'd like to hear the justification. I see any custom icon as being equivalent to the vanilla map icons, since they are not curses, and liable to just be rendered in order.

If we dropped priority and moved to a registration setup, we'd have the option of a single std::vector<MinimapIcon>, where the struct has a boolean to determine if it should be rendered at the moment. Not saying we have to do that, just a thought.

This is all subjective of course, but i do think that letting users change the placement is useful.

For my own usecase I'd like to place the icons right before the curses, but after all of the item ones. But if a user makes an item that reveals the map in some way it might make more sense to place it before the restock one, together with the other map related icons. (Think of a "red map" item, similar to blue map but revealing the ultra secret room)

However, since we're moving to a registration system, we may be able to keep the priority and only use a single std::vector.

Guantol-Lemat · 2025-12-14T19:18:19Z

repentogon/LuaInterfaces/HUD/LuaMinimap.cpp

+	int id = (int)luaL_checkinteger(L, 1);
+
+	if (id < 0 || id >= RegisteredCustomIcons.size()) {
+		return 0;


This and other Lua functions that need input validation other than type, should trigger a lua arg error, and not silently fail.

Guantol-Lemat · 2025-12-14T21:29:41Z

repentogon/LuaInterfaces/HUD/LuaMinimap.cpp

+static std::set<int> ActiveCustomIcons;
+
+#pragma region Helper functions
+void TryAddIcon(MinimapIcon icons[], unsigned int& num_icons, ANM2* sprite, std_string& anim, int frame) {


I suggest marking all functions that do not need to be accessed outside of this file as static, avoiding potential linker errors due to name clashes.

Guantol-Lemat · 2025-12-14T21:36:00Z

repentogon/LuaInterfaces/HUD/LuaMinimap.cpp

+	return true;
+}
+
+bool CompareIcons(RegisteredMinimapIcon* a, RegisteredMinimapIcon* b) {


I would personally rename this function, as this specifically tells you if the map has a lower position, rather than being a three-way comparison, alternatively you could define an operator< in the structure, so std::lower_bound can use it automatically, or use a lambda if this comparison is only meant to be used during sorting.

To provide examples for the alternative suggestions:

Struct with less-than operator (enables direct comparison of the struct, and natural use of things like std::lower_bound without specifying a comparator):

struct RegisteredCustomIcons { bool operator<(const RegisteredMinimapIcon& other) const { return position < other.position; } };

Lambda (essentially an inline function):

std::lower_bound(RegisteredCustomIcons.begin(), RegisteredCustomIcons.end(), minimapIcon, [](const RegisteredMinimapIcon& a, const RegisteredMinimapIcon& b) { return a.position < b.position; });

The lambda example can be nice because then you have the logic inline within the context of the function. If you never need to do such a comparison ANYWHERE else in the code, the lambda might be a good option to take, since you don't need the function elsewhere.

(Note I did not test these examples, I could have made a mistake somewhere but the general structure is correct)

Guantol-Lemat · 2025-12-14T21:43:54Z

repentogon/LuaInterfaces/HUD/LuaMinimap.cpp

+	int id;
+};
+
+static std::vector<RegisteredMinimapIcon*> RegisteredCustomIcons;


Why use pointers to the structures when this container is supposed to be their owner?

If I don't use pointers here I get access violation errors when trying to access these elements in other functions.

Is there anything that im doing wrong in the struct initialization that I should change to be able to not use pointers?

This does not need to be pointers, you could definitely have std::vector<RegisteredMinimapIcon>

I don't immediately know the exact reason you are getting access violations, but one possible guess I have is that you are creating a copy of your struct and the ANM2 object is not being handled properly (ie, you create a copy of the contained ANM2 when you insert into the vector, but it just copies some of the contained pointers which break when the original is deleted).

While it is possible for us to make our ANM2s copyable, that's not really the point here, since you want to avoid unnecessary copies anyway. We do not need to copy an ANM2 at any point.

One way to handle this, I THINK, would be to do std::move(minimapIcon) when inserting it into the structure, which moves the pre-created RegisteredMinimapIcon directly into the structure. This immediately invalidates the local minimapIcon variable, but that is ok if the insertion is virtually the last thing done in the function. Don't try to use a variable after std::moveing it

Typically you'd create the structure directly during insertion, by either constructing inline with push_back or using emplace with a constructor, but in this case you want to build the struct first to validate the loaded anm2 BEFORE inserting it, so a std::move might be reasonable.

Functionally, to be fair, the use of a pointer here is okay since we never need to free it, but its still better practice to maintain clear ownership of data, so std::vector<RegisteredMinimapIcon> feels better.

Attempted using std::move and constructing the struct directly when inserting into the vector (using emplace and insert), but they all result in an access violation.

I pinpointed the crash the rendering loop, where the pointer to the sprite is used, so I'm guessing doing &registeredIcon.sprite makes a weird pointer somehow. (??)

You are most likely still getting tripped up on some C++ nitty gritty somewhere along the line that is a bit hard to determine second hand. I don't believe there's a fundamental issue with the structs being owned by the vector. Maybe there is still an unintentional copy happening somewhere.

Personally, I would be okay if you leave it as a pointer for now, strictly because we never need to free a registered icon. After you've finished with other code changes in this review, potentially even after we merge this in, we can take a closer look at this particular bit (I mostly just don't want to get stuck in a back-and-forth for this issue while you're making other changes, and it does work fine as-is even if not ideal)

ConnorForan · 2025-12-15T01:37:45Z

repentogon/LuaInterfaces/HUD/LuaMinimap.cpp

+
+struct MinimapIcon {
+	ANM2* sprite;
+	std_string anim;


Since the MinimapIcon struct is separate from the RegisteredMinimapIcon struct, then having the anm2 here as a pointer makes sense since the ANM2 is owned by either the game (for vanilla icons) or by the RegisteredMinimapIcon (for custom ones)

So this seems okay as is rn, it being generic for both vanilla and custom icons makes sense.

ConnorForan · 2025-12-15T02:00:36Z

repentogon/LuaInterfaces/HUD/LuaMinimap.cpp

+	int id;
+};
+
+static std::vector<RegisteredMinimapIcon*> RegisteredCustomIcons;


This does not need to be pointers, you could definitely have std::vector<RegisteredMinimapIcon>

I don't immediately know the exact reason you are getting access violations, but one possible guess I have is that you are creating a copy of your struct and the ANM2 object is not being handled properly (ie, you create a copy of the contained ANM2 when you insert into the vector, but it just copies some of the contained pointers which break when the original is deleted).

While it is possible for us to make our ANM2s copyable, that's not really the point here, since you want to avoid unnecessary copies anyway. We do not need to copy an ANM2 at any point.

One way to handle this, I THINK, would be to do std::move(minimapIcon) when inserting it into the structure, which moves the pre-created RegisteredMinimapIcon directly into the structure. This immediately invalidates the local minimapIcon variable, but that is ok if the insertion is virtually the last thing done in the function. Don't try to use a variable after std::moveing it

Typically you'd create the structure directly during insertion, by either constructing inline with push_back or using emplace with a constructor, but in this case you want to build the struct first to validate the loaded anm2 BEFORE inserting it, so a std::move might be reasonable.

Functionally, to be fair, the use of a pointer here is okay since we never need to free it, but its still better practice to maintain clear ownership of data, so std::vector<RegisteredMinimapIcon> feels better.

ConnorForan · 2025-12-15T02:05:17Z

repentogon/LuaInterfaces/HUD/LuaMinimap.cpp

+	Vector vector_zero(0, 0);
+
+	unsigned int num_icons = 0;
+	MinimapIcon icons_to_render[MAX_MINIMAP_ICONS];


I feel like this could just be another std::vector and use .size() to check the current num_icons

TryAddIcon etc would receive it as std::vector<MinimapIcon>& icons_to_render (ie, a mutable reference)

One way to add to the vector without any copying would be vec.push_back(MinimapIcon{ sprite, anim, frame });

ConnorForan · 2025-12-15T02:07:12Z

repentogon/LuaInterfaces/HUD/LuaMinimap.cpp

+	}
+}
+
+void TryAddAllIcons(MinimapIcon icons[], unsigned int& num_icons, std::set<int>::iterator& it, int position) {


Could probably all this "TryAddCustomIcons", since it is specific to that

ConnorForan · 2025-12-15T02:24:49Z

repentogon/LuaInterfaces/HUD/LuaMinimap.cpp

+	return true;
+}
+
+bool CompareIcons(RegisteredMinimapIcon* a, RegisteredMinimapIcon* b) {


To provide examples for the alternative suggestions:

Struct with less-than operator (enables direct comparison of the struct, and natural use of things like std::lower_bound without specifying a comparator):

struct RegisteredCustomIcons { bool operator<(const RegisteredMinimapIcon& other) const { return position < other.position; } };

Lambda (essentially an inline function):

std::lower_bound(RegisteredCustomIcons.begin(), RegisteredCustomIcons.end(), minimapIcon, [](const RegisteredMinimapIcon& a, const RegisteredMinimapIcon& b) { return a.position < b.position; });

The lambda example can be nice because then you have the logic inline within the context of the function. If you never need to do such a comparison ANYWHERE else in the code, the lambda might be a good option to take, since you don't need the function elsewhere.

(Note I did not test these examples, I could have made a mistake somewhere but the general structure is correct)

ConnorForan · 2025-12-15T02:41:19Z

repentogon/LuaInterfaces/HUD/LuaMinimap.cpp

+	minimapIcon->sprite.Load(anm2_string, true);
+
+	if (!ValidateSprite(minimapIcon->sprite, anim, frame)) {
+		lua_pushnil(L);


For some reason I'm thinking around this validation and the potential to return nil. I'm actually not certain what I prefer here (might be OK to leave as is, just uncertain) and I am open to thoughts from both of you, something just feels slightly off to me.

One other option we have would be to allow registration regardless, but if the anm2/anim/frame isn't valid, the icon won't render, or will appear to be "blank". I feel like there are other situations where messing up your anm2s will lead to things just being invisible instead of erroring outright, and perhaps this would be consistent with that. The mod creator could still tell something is up and might suspect the anm2. There's no fail state for registration itself, its just the sprite info isn't valid for rendering.

Another alternative is that we return -1 here instead. This feels somewhat appropriate, and that value would still either do nothing (or error) when sent to the other functions.

Thinking back I'd remove the validation too. It's more consistent with other vanilla behaviour, as you mentioned, and it's inmediately noticeable to the user.

With that in mind I don't really think that returning -1 is a good idea, since that would cause other functions to silently fail, without letting the user know, unless we error. But then again, having a blank icon accomplishes the same thing while being more consistent.

While registration-like functions are not common in the API right now, most functions that return IDs will return something like -1 for invalid things. And in general, return values being userdata or nil is common, but number or nil is not. While its not offensive to lua, it still feels odd. I also don't feel like theres any reason -1 would be worse for error handling, since it's an invalid ID anyway. I'm not completely against returning nil, but I think in general the whole concept was new/unique enough that I wasn't certain of my preferred pattern.

But again, maybe just removing the validation and allowing rendering to simply not work may be simpler here.

Added custom map icon support

d2c0b5a

Guantol-Lemat reviewed Dec 12, 2025

View reviewed changes

ConnorForan reviewed Dec 13, 2025

View reviewed changes

Changed to icon registration

28a3d85

Guantol-Lemat reviewed Dec 14, 2025

View reviewed changes

ConnorForan reviewed Dec 15, 2025

View reviewed changes

Custom map icons support #788

Are you sure you want to change the base?

Custom map icons support #788

Uh oh!

Conversation

thicco-catto commented Dec 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment