Skip to content
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.

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

Quality of life: Spellcasting #536

Merged
merged 21 commits into from
Jan 23, 2025
Merged

Conversation

NQNStudios
Copy link
Collaborator

This makes changes to the spellcasting UI.

  • M or P to recast will no longer default to Light or Minor Bless/Minor Heal. You need to cast something before recast becomes available. This fixes load party, press P: "Priest spell Light not implemented for town mode." #535 and I think it's disorienting when I've just started the game and M casts Light in a town that's fully lit, so the change is generally good I'd say.
  • I implemented a recasting hint in the text bar, which was one of the things I mentioned in my quality-of-life checklist Quality of life wishlist NQNStudios/cboe#16. It should move status icons to the left when necessary, to fall between the left text and right text. I think with very long spell names, this could overlap with the text, which would be bad. But I don't know what statuses go in the text bar so I haven't tested/handled that case yet.
Screenshot 2025-01-18 at 5 39 31 PM
  • Sometimes when my eyes glaze over, I think I'm casting the spell on the wrong side of the LED. I thought there was a bug when I cast Long Light instead of Dumbfound (even though I know the distance between the two is pretty large -- I wasn't paying much attention). I thought it would be nice to highlight the name of the selected spell. Light green seemed to make more sense than red for that, because the LED turns green. Then I made the caster/target selection texts also use light green instead of red, to match. Uncastable spells are grey.
Screenshot 2025-01-18 at 5 39 15 PM

Honestly still haven't decided if I like how it looks, so feedback is welcome.

@NQNStudios
Copy link
Collaborator Author

  • It should move status icons to the left when necessary, to fall between the left text and right text. I think with very long spell names, this could overlap with the text, which would be bad. But I don't know what statuses go in the text bar so I haven't tested/handled that case yet.

The thing to do about this would probably be to calculate how much space is taken up by the name, APs, and active status icons, then squish the spell hint horizontally as needed.

@CelticMinstrel
Copy link
Member

CelticMinstrel commented Jan 20, 2025

Hmm… my first reaction to the spell hint was "that sounds bad, there just isn't enough space", and I still mostly feel that way (what about characters that can cast both mage and priest spells)? However, if it's only shown as part of the combat AP indicator, then I think I can live with it. In that case we can drop the status icons altogether – the only statuses shown there are whole-party statuses (fly, detect life, firewalk, stealth), most of which have no relevance in combat. (I think only firewalk could have combat relevance?)

So, to be clear: in combat we can show this indicator, and outside of combat we can show the status icons, but not both at the same time. Does that work? I'm not sure if it undermines the intent of your hint.

Oh, and about the redesign of the spellcasting dialogue: I like it.

While you're at it, maybe you can fix the alignment of the headers? Like, aligning "HP" with the column of HP, "SP" with the column of SP, etc.

@NQNStudios
Copy link
Collaborator Author

Here's a redesign of the spell window that aligns the numbers, buttons, statuses etc. and colorizes HP/SP numbers

Screenshot 2025-01-20 at 1 22 45 PM

parseColor() was doing a big if-else with string comparisons,
and returning values that differed from our Colours constants.
Since I couldn't find an instance of the colour attribute in our
existing xml, it should be safe to do away with those conflicting
values and refactor parseColor() to match the constants
and use cleaner code.
@NQNStudios
Copy link
Collaborator Author

So, to be clear: in combat we can show this indicator, and outside of combat we can show the status icons, but not both at the same time. Does that work? I'm not sure if it undermines the intent of your hint.

I think this is just fine.

(what about characters that can cast both mage and priest spells)?

I've just made it so attempting to recast the opposite spell type of the hint, toggles the hint to the other spell type, so you get to see it before repeating the input to do the recast.

I made this part of the spellcasting action that we already record and replay, so it should work in replays.

I found it very confusing how many of the spells were
(WHEN_COMBAT).asPeaceful(). I thought those were bugs at first.
It seemed reasonable that we actually would want to LET
the player cast this in town mode, but it seems like other
blessing spells (i.e. Haste) do not
@CelticMinstrel
Copy link
Member

Regarding the colours, I think my logic behind choosing those colours was that they are the basic HTML colour set… or perhaps it was that it's the basic Win32 console colour set, or the union of the two. So I think I'd prefer to keep all the colours that were defined before, though moving them to a colour map definitely makes the code cleaner.

One other note on the colours – for two-word colours, could we replace the underscores with hyphens? So for example color="title-blue" instead of color="title_blue".

Also, I think you missed updating the schema for the colours change? Unless I missed it.


I notice you added a documentation comment in the cpp (asPeaceful). Wouldn't that make more sense in the header?

@NQNStudios
Copy link
Collaborator Author

Regarding the colours, I think my logic behind choosing those colours was that they are the basic HTML colour set… or perhaps it was that it's the basic Win32 console colour set, or the union of the two. So I think I'd prefer to keep all the colours that were defined before, though moving them to a colour map definitely makes the code cleaner.

For the ones that exist as Colours constants can I use those values instead of the ones that were defined before? It'd be really weird/inconvenient if the xml couldn't make controls match colors with the programmatic values

@NQNStudios
Copy link
Collaborator Author

Also, I think you missed updating the schema for the colours change? Unless I missed it.

color/colour actually doesn't have its acceptable values specified in the schema.

@NQNStudios
Copy link
Collaborator Author

NQNStudios commented Jan 22, 2025

I notice you added a documentation comment in the cpp (asPeaceful). Wouldn't that make more sense in the header?

I think it would actually make the most sense right next to the declarations of all the spells. I added that note in because I thought all the instances of when(WHEN_COMBAT).asPeaceful() were bugs and they're not.

So I put it there.

@CelticMinstrel
Copy link
Member

CelticMinstrel commented Jan 22, 2025

For the ones that exist as Colours constants can I use those values instead of the ones that were defined before? It'd be really weird/inconvenient if the xml couldn't make controls match colors with the programmatic values

How different were they? If they're reasonably similar colours then the version in Colours should be fine.

color/colour actually doesn't have its acceptable values specified in the schema.

Hmm, maybe that should be added, but if it's already not there then it won't block this PR.

Also, you misspelled "silver".

@NQNStudios
Copy link
Collaborator Author

How different were they? If they're reasonably similar colours then the version in Colours should be fine.

They're pretty similar, and also nothing was referencing the variants that would have come from the old parseColor() so there's no functional change.

Also, you misspelled "silver".

That I did. Funny that it wasn't fuchsia that tripped me up!

@CelticMinstrel
Copy link
Member

Okay, I did a comparison of the colours. The ones that are different are:

  • red and blue use 0xdd instead of 0xff
  • yellow adds 0x31 blue to mellow it out a bit
  • navy is even darker - 0x39 instead of 0x80
  • teal is a significantly different colour

The differences mostly aren't noticeable unless you look closely (except for teal). I did wonder if perhaps some of the "extra" colours that use 0xff should instead either use 0xdd or fill the gap with 0x31, so I tried this out:

  • lime with 0xdd is noticeably darker but still quite bright
  • aqua as 00dddd is noticeably darker but still quite bright
  • aqua as 00ffdd is about the same as 00ffff while 00ddff is about the same as 00dddd
  • fuchsia as dd00dd is noticeably darker but still quite bright
  • fuchsia as ff00dd is about the same as ff00ff while dd00ff is about the same as dd00dd
  • aqua as 31dddd doesn't look noticeably different to me
  • fuchsia as dd31dd doesn't look noticeably different to me.

I think they're probably fine as they are, but if you think any of them are overly bright then you could try reducing them as above.

@NQNStudios
Copy link
Collaborator Author

NQNStudios commented Jan 23, 2025 via email

@CelticMinstrel CelticMinstrel merged commit 2ec456b into calref:master Jan 23, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

load party, press P: "Priest spell Light not implemented for town mode."
2 participants