Skip to content

Custom map icons support #788

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
thicco-catto wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Custom map icons support #788

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d2c0b5a
Added custom map icon support
thicco-catto Dec 12, 2025
28a3d85
Changed to icon registration
thicco-catto Dec 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions libzhl/functions/Game.zhl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,9 @@ static Entity_Effect* Game::ChainLightning(Vector* position, float damage<xmm2>,
"558bec83ec0c568bb1":
__thiscall bool Game::HasSeedEffect(int SeedEffect);

"a1????????56578bb0????????0b71??e8????????8bf80bfee8????????f7d023c75f5e":
__thiscall int Game::GetCurses();

"a1(????????)c3????????????????????558bec6a00":
reference Game *g_Game;

Expand Down
256 changes: 256 additions & 0 deletions repentogon/LuaInterfaces/HUD/LuaMinimap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
#include "IsaacRepentance.h"
#include "LuaCore.h"
#include "HookSystem.h"
#include <memory>
#include "../../Patches/CustomModManager.h"

/*LUA_FUNCTION(Lua_GameGetMinimap)
{
Expand All @@ -12,6 +14,257 @@
}
*/

enum MinimapIconPosition {
ICON_POSITION_BEFORE_MAP_ICONS = 0,
ICON_POSITION_AFTER_MAP_ICONS = 1,
ICON_POSITION_BEFORE_CURSE_ICONS = 2,
ICON_POSITION_AFTER_CURSE_ICONS = 3,
};

const int MAX_MINIMAP_ICONS = 7;
const int BASE_ICON_X_OFFSET = 16;
const int MAX_CURSES = 8;

struct MinimapIcon {
ANM2* sprite;
std_string anim;

@Guantol-Lemat Guantol-Lemat Dec 12, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ConnorForan ConnorForan Dec 13, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@thicco-catto thicco-catto Dec 14, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Guantol-Lemat Guantol-Lemat Dec 14, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ConnorForan ConnorForan Dec 15, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

int frame;
};

struct RegisteredMinimapIcon {
ANM2 sprite;
int position;
// Unique id that's passed to the lua side
int id;
};

static std::vector<RegisteredMinimapIcon*> RegisteredCustomIcons;

@Guantol-Lemat Guantol-Lemat Dec 14, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@thicco-catto thicco-catto Dec 14, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@ConnorForan ConnorForan Dec 15, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@thicco-catto thicco-catto Dec 15, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. (??)

@ConnorForan ConnorForan Dec 15, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

// Set containing the indexes in RegisteredCustomIcons containing active icons
static std::set<int> ActiveCustomIcons;

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

@Guantol-Lemat Guantol-Lemat Dec 14, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (num_icons < MAX_MINIMAP_ICONS) {
icons[num_icons].sprite = sprite;
icons[num_icons].anim = anim;
icons[num_icons].frame = frame;

num_icons++;
}
}

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

@ConnorForan ConnorForan Dec 15, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (num_icons >= MAX_MINIMAP_ICONS) {
return;
}

for (it; it != ActiveCustomIcons.end(); ++it) {
int icon_idx = *it;
RegisteredMinimapIcon* icon = RegisteredCustomIcons.at(icon_idx);

if (icon->position <= position) {
TryAddIcon(icons, num_icons, &icon->sprite, icon->sprite._animData->_name, icon->sprite.GetFrame());
} else {
break;
}

if (num_icons >= MAX_MINIMAP_ICONS) {
break;
}
}
}

bool ValidateSprite(ANM2& sprite, const char* anim, int& frame) {
if (!sprite._loaded) {
return false;
}

sprite.SetAnimation(anim, true);
AnimationData* anim_data = sprite._animData;
if (anim_data == nullptr) {
return false;
}

if (frame >= anim_data->GetLength()) {
return false;
}

return true;
}

bool CompareIcons(RegisteredMinimapIcon* a, RegisteredMinimapIcon* b) {

@Guantol-Lemat Guantol-Lemat Dec 14, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ConnorForan ConnorForan Dec 15, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

return a->position < b->position;
}
#pragma endregion

HOOK_METHOD(Game, Exit, (bool shouldSave) -> void) {
ActiveCustomIcons.clear();

super(shouldSave);
}

HOOK_METHOD(Minimap, RenderIcons, (Vector* position, float alpha) -> void) {
// Check if we actually need to do the custom logic.
if (ActiveCustomIcons.size() <= 0) {
super(position, alpha);
return;
}

PlayerManager* player_manager = g_Game->GetPlayerManager();
RNG* rng = &player_manager->_rng;
Vector vector_zero(0, 0);

unsigned int num_icons = 0;
MinimapIcon icons_to_render[MAX_MINIMAP_ICONS];

@ConnorForan ConnorForan Dec 15, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 });

Vector render_position(position->x, position->y);

std_string icons_anim = "icons";
std_string curses_anim = "curses";

std::set<int>::iterator active_custom_icons_it = ActiveCustomIcons.begin();

#pragma region Item icons
TryAddAllIcons(icons_to_render, num_icons, active_custom_icons_it, ICON_POSITION_BEFORE_MAP_ICONS);

if (player_manager->FirstCollectibleOwner(COLLECTIBLE_MIND, &rng, true)) {
TryAddIcon(icons_to_render, num_icons, &this->_itemIconsSprite, icons_anim, 5);
} else {
if (player_manager->FirstCollectibleOwner(COLLECTIBLE_COMPASS, &rng, true)) {
TryAddIcon(icons_to_render, num_icons, &this->_itemIconsSprite, icons_anim, 0);
}

if (player_manager->FirstCollectibleOwner(COLLECTIBLE_BLUE_MAP, &rng, true)) {
TryAddIcon(icons_to_render, num_icons, &this->_itemIconsSprite, icons_anim, 1);
}

if (player_manager->FirstCollectibleOwner(COLLECTIBLE_TREASURE_MAP, &rng, true)) {
TryAddIcon(icons_to_render, num_icons, &this->_itemIconsSprite, icons_anim, 2);
}
}

TryAddAllIcons(icons_to_render, num_icons, active_custom_icons_it, ICON_POSITION_AFTER_MAP_ICONS);

if (player_manager->FirstCollectibleOwner(COLLECTIBLE_RESTOCK, &rng, true)) {
TryAddIcon(icons_to_render, num_icons, &this->_itemIconsSprite, icons_anim, 4);
}
#pragma endregion

#pragma region Curse icons
TryAddAllIcons(icons_to_render, num_icons, active_custom_icons_it, ICON_POSITION_BEFORE_CURSE_ICONS);

unsigned int curses = g_Game->GetCurses();

for (size_t i = 0; i < MAX_CURSES; i++) {
if ((curses & 1 << i) != 0) {
TryAddIcon(icons_to_render, num_icons, &this->_itemIconsSprite, curses_anim, i);
}
}

std::map<uint32_t, CurseSpriteData> curseSpriteMap = GetCurseSpriteMap();

if (!curseSpriteMap.empty() && num_icons < MAX_MINIMAP_ICONS) {
for (const auto& entry : curseSpriteMap) {
int adjustedCurseId = (int)entry.first;
ANM2* curseSprite = entry.second.customANM2;

if (adjustedCurseId >= (MAX_CURSES + 1) && curseSprite->_loaded && (curses & (1U << (adjustedCurseId - 1)))) {
TryAddIcon(icons_to_render, num_icons, curseSprite, curses_anim, entry.second.frameNum);

if (num_icons >= MAX_MINIMAP_ICONS) break;
}
}
}

TryAddAllIcons(icons_to_render, num_icons, active_custom_icons_it, ICON_POSITION_AFTER_CURSE_ICONS);
#pragma endregion

#pragma region Rendering
Vector offset(-BASE_ICON_X_OFFSET, 0);
if (num_icons >= 4) {
offset += Vector(num_icons - 3.0f, 0);
}

for (size_t i = 0; i < num_icons; i++) {
MinimapIcon icon = icons_to_render[i];

icon.sprite->SetFrame(&icon.anim, icon.frame);

icon.sprite->_color._tint[3] = alpha;
icon.sprite->Render(&render_position, &vector_zero, &vector_zero);

render_position += offset;
}
#pragma endregion
}

LUA_FUNCTION(Lua_MinimapRegisterCustomIcon) {
int position = (int)luaL_checkinteger(L, 1);
const char* anm2 = luaL_checkstring(L, 2);
std_string anm2_string(anm2);
const char* anim = luaL_checkstring(L, 3);
std_string anim_string(anim);
int frame = (int)luaL_checkinteger(L, 4);

RegisteredMinimapIcon* minimapIcon = new RegisteredMinimapIcon;
minimapIcon->position = position;

minimapIcon->sprite.Load(anm2_string, true);

if (!ValidateSprite(minimapIcon->sprite, anim, frame)) {
lua_pushnil(L);

@ConnorForan ConnorForan Dec 15, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@thicco-catto thicco-catto Dec 15, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ConnorForan ConnorForan Dec 15, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {
minimapIcon->sprite.SetFrame(&anim_string, frame);

minimapIcon->id = RegisteredCustomIcons.size();
int index_to_place_at = 0;

auto lb = std::lower_bound(RegisteredCustomIcons.begin(), RegisteredCustomIcons.end(), minimapIcon, CompareIcons);
RegisteredCustomIcons.insert(lb, minimapIcon);

lua_pushinteger(L, minimapIcon->id);
}

return 1;
}

LUA_FUNCTION(Lua_MinimapActivateCustomIcon) {
int id = (int)luaL_checkinteger(L, 1);

if (id < 0 || id >= RegisteredCustomIcons.size()) {
return 0;

@Guantol-Lemat Guantol-Lemat Dec 14, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

for (size_t i = 0; i < RegisteredCustomIcons.size(); i++)
{
RegisteredMinimapIcon* icon = RegisteredCustomIcons.at(i);
if (icon->id == id) {
ActiveCustomIcons.insert(i);
return 0;
}
}

return 0;
}

LUA_FUNCTION(Lua_MinimapDeactivateCustomIcon) {
int id = (int)luaL_checkinteger(L, 1);

if (id < 0 || id >= RegisteredCustomIcons.size()) {
return 0;
}

for (size_t i = 0; i < RegisteredCustomIcons.size(); i++)
{
RegisteredMinimapIcon* icon = RegisteredCustomIcons.at(i);
if (icon->id == id) {
ActiveCustomIcons.erase(i);
return 0;
}
}

return 0;
}

LUA_FUNCTION(Lua_MinimapGetState)
{
Minimap* minimap = g_Game->GetMinimap();
Expand Down Expand Up @@ -115,6 +368,9 @@ static void RegisterMinimap(lua_State* L) {
lua::TableAssoc(L, "GetShakeOffset", Lua_MinimapGetShakeOffset);
lua::TableAssoc(L, "SetShakeOffset", Lua_MinimapSetShakeOffset);
lua::TableAssoc(L, "Refresh", Lua_MinimapRefresh);
lua::TableAssoc(L, "RegisterIcon", Lua_MinimapRegisterCustomIcon);
lua::TableAssoc(L, "ActivateIcon", Lua_MinimapActivateCustomIcon);
lua::TableAssoc(L, "DeactivateIcon", Lua_MinimapDeactivateCustomIcon);
//};
lua_setglobal(L, "Minimap");
//lua::RegisterNewClass(L, lua::metatables::MinimapMT, lua::metatables::MinimapMT, functions);
Expand Down
13 changes: 6 additions & 7 deletions repentogon/Patches/CustomModManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,6 @@

#include "CustomModManager.h"

struct CurseSpriteData {
ModEntryEx* modEx;
ANM2* customANM2;
uint32_t frameNum;
};

static std::map<uint32_t, CurseSpriteData> curseSpriteMap;
static ModEntry* lastModEntry = nullptr;
static uint32_t currentFrameNum = 0;
Expand Down Expand Up @@ -196,4 +190,9 @@ void ASMPatchesForCustomModManager() {
ASMPatchRegisterCurseSprite();
ASMPatchAssignCustomFrame();
ASMPatchRenderCustomCurses();
}
}

std::map<uint32_t, CurseSpriteData> GetCurseSpriteMap()
{
return curseSpriteMap;
}
10 changes: 9 additions & 1 deletion repentogon/Patches/CustomModManager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,12 @@ class CustomModManager
CustomModManager& operator=(const CustomModManager&) = delete;
};

void ASMPatchesForCustomModManager();
struct CurseSpriteData {
ModEntryEx* modEx;
ANM2* customANM2;
uint32_t frameNum;
};

void ASMPatchesForCustomModManager();

std::map<uint32_t, CurseSpriteData> GetCurseSpriteMap();
7 changes: 7 additions & 0 deletions repentogon/resources/scripts/enums_ex.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3114,6 +3114,13 @@ RoomDisplayFlags = {
SHOW_ICON = 1 << 2,
}

MinimapIconPosition = {
BEFORE_MAP_ICONS = 0,
AFTER_MAP_ICONS = 1,
BEFORE_CURSE_ICONS = 2,
AFTER_CURSE_ICONS = 3
}

--deprecated enums

Achievement.REVERSED_THE_HEIROPHANT = 529
Expand Down
Loading