Improve invlet management for Magiclysm spell list.

[maciekk]

First, impose sensible order on spell list. Specifically, we sort by 3 dimensions:

1. favorite spells before non-favorite
2. by invlet
3. by spell name (in case no invlet; not currently possible)

Second, this change fixes some issues:

• make auto-assigned invlets "permanent" (vs scrambling letters when spells are added or subtracted)
• actually properly check if the desired invlet is actually in use (vs only checking against "reserved")

Motivation:

• addresses expected behavior; e.g., https://www.reddit.com/r/cataclysmdda/comments/oq2g3s/any_way_to_change_spell_list_order/
• might actually address Issue 50465
• might partially mitigate Issue 50018

Summary

Bugfixes "Better handling of spell list (sorted, bug fixes)"

Purpose of change

Primary purpose is to sort the Magiclysm spell list.

Secondary, wider purpose is to remove some adjacent bugs / warts that testing revealed (e.g., you can assign the same letter to multiple spells), so as to end up with a more satisfactory user experience with the list.

Describe the solution

For sorting the list, we use 3 sort "keys", in order:

• sort favorited spells before non-favorited
• sort by invlet (using invchars.get_allowed_letters(), rather than pure alpha... because want 'a' before 'A', for example)
• sort by spell name (not actually used yet, since all spells get an invlet... but down the road, if we respect the "auto assign invlet" option, for example, this will be useful)

NOTE: I realise style guidelines discourage larger lambdas, but I think it is warranted here, because in C++ we cannot define function-with-function otherwise, and we do need much context. So I think it's warranted here, because ultimately it leads to clearer, easier to understand code.

The bug fixes are more on the trivial side.

Describe alternatives you've considered

1. sorting could be done by constructing a "key", where we smush together key pieces of each sorting dimension, and then sort a vector of said keys (paired with spell list indices, etc)... this was initial approach, but felt hacky, was lengthier, and had a number of other draw backs. Current version is cleaner.

2. the invlet handling here feels generally brittle, especially the used_invlet tracking which essentially feels like a cache mechanism of invlets[spell_ids], and can easily get unsynchronized... the whole handling of used_invlet should likely just get encapsulated, but am trying to keep scope of this PR smallish. In general, this code seems brittle...

Testing

1. start character with many spells already known
2. bring up spell list
3. for each spell, delete binding (assign letter, but then use Backspace, or other non-allowed letter)
4. re-open spell list window, notice how spells are assigned a, b, c, d, etc... and listed in order
5. pick a spell from early on, and assign it 'z'
6. re-open spell list and confirm it is at end now
7. pick a spell from mid-pack, favorite it
8. re-open spell list and confirm it is listed at start now

Additional context

FWIW, this is my first PR, for CDDA, or anywhere else on GitHub, so apologies up front on any fumbling anything. Suggestions welcome. Also, this PR is also a way for me to start getting familiar with CDDA code, and style, so any suggestions in this department would be welcome as well.

[maciekk]

Just checked in on the failing build (GCC 9, Curses, LTO), but I don't think it's the change... it hit "lto-wrapper: fatal error: write: No space left on device"

[Maleclypse]

Just checked in on the failing build (GCC 9, Curses, LTO), but I don't think it's the change... it hit "lto-wrapper: fatal error: write: No space left on device"

LTO is broken for a number of reasons and it's not required to pass only tests labeled required are needed to be mergeable. Though Clang 17 should be listed as required.

[maciekk]

Fixed the other compiler complaint, including a fix to my fix (when making the 'auto' explicit)...

I'm starting to think it didn't like the fact the for() variable was a reference... IIRC most compilers will implicitly convert between char and int, but presumably if you have a reference, the types must match exactly (as the implicit conversion possibly introduces a temp variable, so you lose the 'reference' aspect)... but I thought the original form had 'auto&' (reference as well), so not sure what's going on...

In any case, explicit everything should solve the issue conclusively.

[Maleclypse]

Hi there is a conflict from a merged PR about imgui in this menu.

[maciekk]

ACK, I'll look at this Tuesday.

On Thu, Aug 15, 2024 at 11:24 PM Maleclypse @.***> wrote:

Hi there is a conflict from a merged PR about imgui in this menu.

— Reply to this email directly, view it on GitHub https://github.com/CleverRaven/Cataclysm-DDA/pull/75624#issuecomment-2292660934, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAH2CZ5MQTFDA5E2A4RU3GDZRVWH3AVCNFSM6AAAAABMMQFE52VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOJSGY3DAOJTGQ . You are receiving this because you authored the thread.Message ID: @.***>

[maciekk]

OK, I think I got it sorted (i.e., now properly merged/rebased onto the recent conflicting change, so conflict resolved), but do let me know if something is not right.

(FWIW, there might some weird history / "push --force" as I initially made a hash of things, with my initial attempt at resolving did not do a rebase, so made it look like a massive diff due to reintroducing all the other commits from others since Aug 11, or some such... I'm still getting familiar with some of these git operations... more familiar with mercurial/hg, some things don't quite translate the same...)

[Maleclypse]

There is still this clang warning 2747 | int invlet = static_cast<int>( ch );

[Maleclypse]

OK, I think I got it sorted (i.e., now properly merged/rebased onto the recent conflicting change, so conflict resolved), but do let me know if something is not right.

(FWIW, there might some weird history / "push --force" as I initially made a hash of things, with my initial attempt at resolving did not do a rebase, so made it look like a massive diff due to reintroducing all the other commits from others since Aug 11, or some such... I'm still getting familiar with some of these git operations... more familiar with mercurial/hg, some things don't quite translate the same...)

No stress you can either selective rebase or I'll squash the commit history.

Edit: Commit history looks good mostly from here. Assuming tests pass I'll merge without squashing.

[Maleclypse]

Thanks for doing this!

[Naadn]

I don't think this PR is ready, it broke a lot of things. it don't recognize the key rebind and after you learn a new spell it will insert the new one into...somewhere, and all key are off when you cast it.

[maciekk]

Hmm, this sounds like steps I have tested... can you give a few steps so I can recreate the issues?

On Sun, Sep 1, 2024 at 2:03 PM Naadn @.***> wrote:

I don't think this PR is ready, it broke a lot of things. it don't recognize the key rebind and after you learn a new spell it will insert the new one into...somewhere, and all key is off.

— Reply to this email directly, view it on GitHub https://github.com/CleverRaven/Cataclysm-DDA/pull/75624#issuecomment-2323444310, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAH2CZ43CDWRHQWCUMFDHIDZUNJG3AVCNFSM6AAAAABMMQFE52VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRTGQ2DIMZRGA . You are receiving this because you authored the thread.Message ID: @.***>

[Aneides126]

I'm having the same problem. Installed fresh and the hotkeys for spells are all messed up. I'm using the No Class Restrictions Mod, if that matters, and starting with Academy Wizard as Last Wizard on Earth, and the hotkeys are messed up for starter spells and later spells - some work right, others cast the wrong spell, others can't even be cast.

[Naadn]

Hmm, this sounds like steps I have tested... can you give a few steps so I can recreate the issues? … On Sun, Sep 1, 2024 at 2:03 PM Naadn @.> wrote: I don't think this PR is ready, it broke a lot of things. it don't recognize the key rebind and after you learn a new spell it will insert the new one into...somewhere, and all key is off. — Reply to this email directly, view it on GitHub <#75624 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAH2CZ43CDWRHQWCUMFDHIDZUNJG3AVCNFSM6AAAAABMMQFE52VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRTGQ2DIMZRGA . You are receiving this because you authored the thread.Message ID: @.>

#76137 has fixed that and merged

[maciekk]

Hi folks, in case this hasn't been reverted yet, then please revert; alas, I won't have time to look at this soon (things got busy in RL)

On Thu, Sep 5, 2024 at 12:39 AM Naadn @.***> wrote:

Hmm, this sounds like steps I have tested... can you give a few steps so I can recreate the issues? … <#m_2260338531166526470_> On Sun, Sep 1, 2024 at 2:03 PM Naadn @.> wrote: I don't think this PR is ready, it broke a lot of things. it don't recognize the key rebind and after you learn a new spell it will insert the new one into...somewhere, and all key is off. — Reply to this email directly, view it on GitHub <#75624 (comment) https://github.com/CleverRaven/Cataclysm-DDA/pull/75624#issuecomment-2323444310>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAH2CZ43CDWRHQWCUMFDHIDZUNJG3AVCNFSM6AAAAABMMQFE52VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRTGQ2DIMZRGA https://github.com/notifications/unsubscribe-auth/AAH2CZ43CDWRHQWCUMFDHIDZUNJG3AVCNFSM6AAAAABMMQFE52VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRTGQ2DIMZRGA . You are receiving this because you authored the thread.Message ID: @.>

#76137 https://github.com/CleverRaven/Cataclysm-DDA/pull/76137 has fixed that and merged

— Reply to this email directly, view it on GitHub https://github.com/CleverRaven/Cataclysm-DDA/pull/75624#issuecomment-2330581362, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAH2CZ3YSIFSVMLOEGYUJSLZU7N7PAVCNFSM6AAAAABMMQFE52VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMZQGU4DCMZWGI . You are receiving this because you authored the thread.Message ID: @.***>