2.0 Beta 1 Pt 2 #697

Merged
NQNStudios merged 101 commits from 2.0b1-b into master 2025-04-09 23:47:51 +00:00
NQNStudios commented 2025-03-16 18:43:59 +00:00 (Migrated from github.com)
No description provided.
CelticMinstrel (Migrated from github.com) reviewed 2025-03-18 23:53:56 +00:00
@@ -941,0 +996,4 @@
break;
}
const cInvenSlot& pick_slot = univ.party[i].has_abil_equip(eItemAbil::LOCKPICKS);
if(!pick_slot){
CelticMinstrel (Migrated from github.com) commented 2025-03-18 23:53:55 +00:00

This doesn't seem great. 6 is used nearly universally to mean "the entire party", and only here that meaning is conveyed by 7 instead?

Though, I will admit that this isn't technically new here – the dialog was already using 6 with a different meaning.

This doesn't seem great. 6 is used nearly universally to mean "the entire party", and _only_ here that meaning is conveyed by 7 instead? Though, I will admit that this isn't technically new here – the dialog was _already_ using 6 with a different meaning.
CelticMinstrel (Migrated from github.com) reviewed 2025-03-19 00:01:05 +00:00
@@ -8,34 +8,15 @@
CelticMinstrel (Migrated from github.com) commented 2025-03-19 00:01:04 +00:00

I think I like using std::tie here.

	int lo, hi;
	std::tie(lo, hi) = status_bounds(which);
I think I like using `std::tie` here. ```suggestion int lo, hi; std::tie(lo, hi) = status_bounds(which); ```
CelticMinstrel (Migrated from github.com) reviewed 2025-03-19 00:11:42 +00:00
@@ -372,34 +372,7 @@ void draw_fields(location where){
Draw_Some_Item(fields_gworld,calc_rect(4,0),terrain_screen_gworld(),where_draw,1,0);
CelticMinstrel (Migrated from github.com) commented 2025-03-19 00:11:41 +00:00

I looked through them. The only other one that seems like it could be moved to the bottom is special spot, though that could just as easily make sense to move to the top.

But aside from that, there are definitely some oddities in the ordering. I would expect crate, barrel, and block to basically be on the same "layer" – generally you only have one of the three on a tile. Probably it makes sense to draw the more "permanent" ones first:

  • barrel
  • crate
  • block
  • web
  • fire and force barrier (the animated ones)
  • quickfire
  • bottom half of force cage (note: the top half is drawn in a completely different place, after characters)

Then the more transient ones on top:

  • Walls (force, ice, blade, fire)
  • Antimagic field
  • Clouds (stinking, sleep)

The one exception to this is the special spot, where it makes sense for it to be visible through anything and thus go on the top… though I'm not sure if it should. Maybe it's better for it to go at the bottom.

I looked through them. The only other one that seems like it could be moved to the bottom is special spot, though that could just as easily make sense to move to the _top_. But aside from that, there are definitely some oddities in the ordering. I would expect crate, barrel, and block to basically be on the same "layer" – generally you only have one of the three on a tile. Probably it makes sense to draw the more "permanent" ones first: * barrel * crate * block * web * fire and force barrier (the animated ones) * quickfire * bottom half of force cage (note: the top half is drawn in a completely different place, after characters) Then the more transient ones on top: * Walls (force, ice, blade, fire) * Antimagic field * Clouds (stinking, sleep) The one exception to this is the special spot, where it makes sense for it to be visible through anything and thus go on the top… though I'm not sure if it _should_. Maybe it's better for it to go at the bottom.
CelticMinstrel (Migrated from github.com) reviewed 2025-03-19 00:27:57 +00:00
@@ -1039,16 +1041,22 @@ short scan_for_response(const char *str) {
return -1;
CelticMinstrel (Migrated from github.com) commented 2025-03-19 00:27:56 +00:00

I think you've missed some here?

The most obvious is DISPEL_FIELD, since the three you do have are also dispels. It's an item-only spell, maybe that's why you missed it.

Then there's unlock and the two versions of move mountains… or are they excluded because they can only be cast in town and this path is only used in combat? Is this only used in combat?

Then there's the summoning spells (of which there are 12 – Sticks to Snakes and Demon are the less obvious ones), which definitely don't target monsters. I don't think it necessarily makes sense to put the summons amongst the enemies, especially since summoning is quite short range. There's also the new Flash Step I added as an item spell.

The field spells are debatable, but I guess you'd normally place them so that enemies are caught in them, so it's fine to have the auto-framing behaviour.

I think you've missed some here? The most obvious is `DISPEL_FIELD`, since the three you _do_ have are also dispels. It's an item-only spell, maybe that's why you missed it. Then there's unlock and the two versions of move mountains… or are they excluded because they can only be cast in town and this path is only used in combat? _Is_ this only used in combat? Then there's the summoning spells (of which there are 12 – Sticks to Snakes and Demon are the less obvious ones), which definitely don't target monsters. I don't think it necessarily makes sense to put the summons amongst the enemies, especially since summoning is quite short range. There's also the new Flash Step I added as an item spell. The field spells are debatable, but I guess you'd normally place them so that enemies are caught in them, so it's fine to have the auto-framing behaviour.
NQNStudios (Migrated from github.com) reviewed 2025-03-19 00:50:07 +00:00
@@ -1039,16 +1041,22 @@ short scan_for_response(const char *str) {
return -1;
NQNStudios (Migrated from github.com) commented 2025-03-19 00:50:06 +00:00

Having recorded a replay with every priest spell yesterday, I can confidently say the code path isn't used for peace-mode priest spells. And that the priest summoning spells don't make you target, they position the summoned creatures arbitrarily.

I should re-check the mage summons because I do think some of them are targeted.

Having recorded a replay with every priest spell yesterday, I can confidently say the code path isn't used for peace-mode priest spells. And that the priest summoning spells don't make you target, they position the summoned creatures arbitrarily. I should re-check the mage summons because I do think some of them are targeted.
NQNStudios (Migrated from github.com) reviewed 2025-03-19 01:05:41 +00:00
@@ -941,0 +996,4 @@
break;
}
const cInvenSlot& pick_slot = univ.party[i].has_abil_equip(eItemAbil::LOCKPICKS);
if(!pick_slot){
NQNStudios (Migrated from github.com) commented 2025-03-19 01:05:40 +00:00

I think it would make sense for cancel to be -1 and all to be 6.

I'd just have to carefully change callers that expect 6 to be a cancellation.

I think it would make sense for cancel to be -1 and all to be 6. I'd just have to carefully change callers that expect 6 to be a cancellation.
CelticMinstrel (Migrated from github.com) reviewed 2025-03-19 01:53:58 +00:00
@@ -1039,16 +1041,22 @@ short scan_for_response(const char *str) {
return -1;
CelticMinstrel (Migrated from github.com) commented 2025-03-19 01:53:56 +00:00

And that the priest summoning spells don't make you target, they position the summoned creatures arbitrarily.

Incorrect. They do position the summoned creatures arbitrarily in town mode, but let you select their positions if you cast in combat. I'm sure of this because all 13 summoning spells (plus Flash Step) are handled in the same place. (I forgot Simulacrum in my earlier count.)

> And that the priest summoning spells don't make you target, they position the summoned creatures arbitrarily. Incorrect. They _do_ position the summoned creatures arbitrarily in town mode, but let you select their positions if you cast in combat. I'm sure of this because all 13 summoning spells (plus Flash Step) are handled in the same place. (I forgot Simulacrum in my earlier count.)
NQNStudios (Migrated from github.com) reviewed 2025-03-19 02:06:46 +00:00
@@ -1039,16 +1041,22 @@ short scan_for_response(const char *str) {
return -1;
NQNStudios (Migrated from github.com) commented 2025-03-19 02:06:46 +00:00

Oh, ok. Well since there's no harm in adding cases here that might not ever be hit, I can just add all the summoning spells, unlock, move mountains, just cast a wide net.

Oh, ok. Well since there's no harm in adding cases here that might not ever be hit, I can just add *all* the summoning spells, unlock, move mountains, just cast a wide net.
NQNStudios (Migrated from github.com) reviewed 2025-03-19 02:07:33 +00:00
@@ -1039,16 +1041,22 @@ short scan_for_response(const char *str) {
return -1;
NQNStudios (Migrated from github.com) commented 2025-03-19 02:07:33 +00:00

Or we could add something to the spell info struct for this.

Or we could add something to the spell info struct for this.
NQNStudios (Migrated from github.com) reviewed 2025-03-19 13:47:12 +00:00
@@ -1039,16 +1041,22 @@ short scan_for_response(const char *str) {
return -1;
NQNStudios (Migrated from github.com) commented 2025-03-19 13:47:12 +00:00

@CelticMinstrel I added a target lock field to cSpell and applied it to every spell in spell.cpp that should do it. For the special spells, I couldn't find spell descriptions but I think I was able to infer whether they'd be targeted towards enemies.

@CelticMinstrel I added a target lock field to cSpell and applied it to every spell in spell.cpp that should do it. For the special spells, I couldn't find spell descriptions but I think I was able to infer whether they'd be targeted towards enemies.
NQNStudios (Migrated from github.com) reviewed 2025-03-19 14:32:55 +00:00
@@ -941,0 +996,4 @@
break;
}
const cInvenSlot& pick_slot = univ.party[i].has_abil_equip(eItemAbil::LOCKPICKS);
if(!pick_slot){
NQNStudios (Migrated from github.com) commented 2025-03-19 14:32:55 +00:00

@CelticMinstrel I set about trying to change this, and I what I found is that the magic numbers and comparisons of return values are so pervasive in the code it feels impossible to change every instance without breaking something. Return values from select_pc() get stored in global state which ends up getting checked in other areas of the code. Lots of variables that get assigned to select_pc() are expecting 6 as their empty value, and getting assigned 6 from other places.

I don't think it's a worthwhile refactor to make.

Also, I see that a lot of places are using 6 to mean not "the entire party", but rather, "any PC in the party", or in other words, a choice that hasn't been made yet but will eventually refer to only 1. 7 here actually means the entire party, and there are no planned game mechanics that will use that feature. It's just a debug thing as it is.

@CelticMinstrel I set about trying to change this, and I what I found is that the magic numbers and comparisons of return values are so pervasive in the code it feels impossible to change every instance without breaking something. Return values from select_pc() get stored in global state which ends up getting checked in other areas of the code. Lots of variables that get assigned to select_pc() are expecting 6 as their empty value, and getting assigned 6 from other places. I don't think it's a worthwhile refactor to make. Also, I see that a lot of places are using 6 to mean not "the entire party", but rather, "any PC in the party", or in other words, a choice that hasn't been made yet but will eventually refer to only 1. 7 here actually means the *entire* party, and there are no planned game mechanics that will use that feature. It's just a debug thing as it is.
CelticMinstrel (Migrated from github.com) reviewed 2025-03-19 17:51:39 +00:00
@@ -941,0 +996,4 @@
break;
}
const cInvenSlot& pick_slot = univ.party[i].has_abil_equip(eItemAbil::LOCKPICKS);
if(!pick_slot){
CelticMinstrel (Migrated from github.com) commented 2025-03-19 17:51:38 +00:00

For what it's worth, there are definitely game mechanics that target the entire party, rather than an as-yet-unspecified member of the party.

For what it's worth, there are _definitely_ game mechanics that target the entire party, rather than an as-yet-unspecified member of the party.
CelticMinstrel (Migrated from github.com) reviewed 2025-03-19 17:54:16 +00:00
@@ -1039,16 +1041,22 @@ short scan_for_response(const char *str) {
return -1;
CelticMinstrel (Migrated from github.com) commented 2025-03-19 17:54:15 +00:00

Just in case: Strengthen Target definitely targets enemies. It's used by a cursed wand that makes your enemy stronger.

I think all the others should be pretty obvious.

Just in case: Strengthen Target definitely targets enemies. It's used by a cursed wand that makes your enemy stronger. I think all the others should be pretty obvious.
NQNStudios commented 2025-03-19 17:54:31 +00:00 (Migrated from github.com)

(sent via email, moved to proper thread)

(sent via email, moved to proper thread)
NQNStudios (Migrated from github.com) reviewed 2025-03-19 18:10:34 +00:00
@@ -941,0 +996,4 @@
break;
}
const cInvenSlot& pick_slot = univ.party[i].has_abil_equip(eItemAbil::LOCKPICKS);
if(!pick_slot){
NQNStudios (Migrated from github.com) commented 2025-03-19 18:10:34 +00:00

But not ones where it’s a choice between 1 and all, no? I can only think of
the “____ All” spells and there’s no selection dialog for those.

But not ones where it’s a choice between 1 and all, no? I can only think of the “____ All” spells and there’s no selection dialog for those.
CelticMinstrel (Migrated from github.com) reviewed 2025-03-20 03:44:30 +00:00
@@ -37,7 +37,24 @@ void reset_item_max();
short item_val(cItem item);
CelticMinstrel (Migrated from github.com) commented 2025-03-20 03:44:29 +00:00

This was originally enum class eSelectPC { ONLY_LIVING, ANY, ONLY_DEAD };, right? Reordering it probably breaks the Select Target special node. Unless you already dealt with that somewhere and I missed it?

This was originally `enum class eSelectPC { ONLY_LIVING, ANY, ONLY_DEAD };`, right? Reordering it probably breaks the Select Target special node. Unless you already dealt with that somewhere and I missed it?
NQNStudios (Migrated from github.com) reviewed 2025-03-20 13:09:27 +00:00
@@ -37,7 +37,24 @@ void reset_item_max();
short item_val(cItem item);
NQNStudios (Migrated from github.com) commented 2025-03-20 13:09:27 +00:00

That didn't need to be fixed, because select target nodes already translated between their own integer mapping and the mode arg to select_pc:

99eb9962b5/src/game/boe.specials.cpp (L2701)

There's no casting integers to the enum type.

That didn't need to be fixed, because select target nodes already translated between their own integer mapping and the mode arg to `select_pc`: https://github.com/NQNStudios/cboe/blob/99eb9962b5063a8bcfbe9adf2b2d4a9874ff6c65/src/game/boe.specials.cpp#L2701 There's no casting integers to the enum type.
CelticMinstrel (Migrated from github.com) reviewed 2025-03-21 00:12:47 +00:00
@@ -12,10 +12,14 @@
#include <algorithm>
CelticMinstrel (Migrated from github.com) commented 2025-03-21 00:12:46 +00:00

This is incorrect. The normal colour is not white. It is white in the game and PC editor and black in the scenario editor. I'm pretty sure I have a defClr or similar somewhere that handles this.

This is incorrect. The normal colour is not white. It is white in the game and PC editor and black in the scenario editor. I'm pretty sure I have a `defClr` or similar somewhere that handles this.
CelticMinstrel (Migrated from github.com) reviewed 2025-03-21 00:16:09 +00:00
CelticMinstrel (Migrated from github.com) commented 2025-03-21 00:16:07 +00:00

I know we don't have localization yet, and plenty of stuff is in an unlocalizable state, but it would be nice if we can avoid adding more stuff that's unlocalizable. In this case, the thing to avoid is subsentence string concatenation. I don't think it's difficult to simply set loop_from_str and loop_to_str to just contain the whole sentence? Then this would become output.setText(loop_from_str + " " + loop_to_str").

I know we don't have localization yet, and plenty of stuff is in an unlocalizable state, but it would be nice if we can avoid adding _more_ stuff that's unlocalizable. In this case, the thing to avoid is subsentence string concatenation. I don't think it's difficult to simply set `loop_from_str` and `loop_to_str` to just contain the whole sentence? Then this would become `output.setText(loop_from_str + " " + loop_to_str")`.
NQNStudios (Migrated from github.com) reviewed 2025-03-21 00:18:54 +00:00
@@ -12,10 +12,14 @@
#include <algorithm>
NQNStudios (Migrated from github.com) commented 2025-03-21 00:18:54 +00:00
	if(!attrs.count("color") && !attrs.count("colour") && getDialog()->getBg() == cDialog::BG_DARK)
		setColour(sf::Color::White);

Is it this, in message.cpp?

``` if(!attrs.count("color") && !attrs.count("colour") && getDialog()->getBg() == cDialog::BG_DARK) setColour(sf::Color::White); ``` Is it this, in message.cpp?
NQNStudios (Migrated from github.com) reviewed 2025-03-21 00:27:55 +00:00
@@ -12,10 +12,14 @@
#include <algorithm>
NQNStudios (Migrated from github.com) commented 2025-03-21 00:27:55 +00:00

oh parent.getDefTextClr()

oh `parent.getDefTextClr()`
CelticMinstrel commented 2025-03-28 01:40:27 +00:00 (Migrated from github.com)

Very low priority, but I'd suggest visually distinguishing the escape button in some way, similar to how the default button is drawn outlined.

Something to open an issue for after merging if you don't think it's worth doing in here.

Very low priority, but I'd suggest visually distinguishing the escape button in some way, similar to how the default button is drawn outlined. Something to open an issue for after merging if you don't think it's worth doing in here.
NQNStudios commented 2025-03-28 02:04:24 +00:00 (Migrated from github.com)

@CelticMinstrel Now that the changes are really starting to stack up (100 commits! maybe my biggest PR?), it might be nice to do some merging and start anew. One of the other PRs has taken commits from this one, and it also stages the tutorial base with arrow damage values that only make sense following #700. So I'd like to see those merged and then rebase this one.

@CelticMinstrel Now that the changes are really starting to stack up (100 commits! maybe my biggest PR?), it might be nice to do some merging and start anew. One of the other PRs has taken commits from this one, and it also stages the tutorial base with arrow damage values that only make sense following #700. So I'd like to see those merged and then rebase this one.
NQNStudios commented 2025-03-28 13:45:02 +00:00 (Migrated from github.com)

Very low priority, but I'd suggest visually distinguishing the escape button in some way, similar to how the default button is drawn outlined.

Something to open an issue for after merging if you don't think it's worth doing in here.

I agree there should be a visual indicator. While we're on that subject, I also don't know if you saw my comment on #67 that I think the existing visual highlight for the default button looks bad on certain backgrounds causing it to be perceived as a bug. I actually think it would be nicer to have color-tinted button sprites, like the ok buttons could be blueish and the cancel buttons could be more reddish.

> Very low priority, but I'd suggest visually distinguishing the escape button in some way, similar to how the default button is drawn outlined. > > Something to open an issue for after merging if you don't think it's worth doing in here. > I agree there should be a visual indicator. While we're on that subject, I also don't know if you saw my comment on #67 that I think the existing visual highlight for the default button looks bad on certain backgrounds causing it to be perceived as a bug. I actually think it would be nicer to have color-tinted button sprites, like the ok buttons could be blueish and the cancel buttons could be more reddish.
CelticMinstrel commented 2025-03-29 13:07:56 +00:00 (Migrated from github.com)

Now that the changes are really starting to stack up (100 commits! maybe my biggest PR?), it might be nice to do some merging and start anew.

Sounds good, but I notice that you have a bunch of fixup commits in here. Did you plan to rebase to squash those?

Also, while I'm here, I think it makes sense to map Ctrl+F to key_find rather than key_none (though key_none can still exist to catch random modifier key combinations). I don't quite remember how the special keys work though so I'm not sure if that affects the XML-set key mapping.

> Now that the changes are really starting to stack up (100 commits! maybe my biggest PR?), it might be nice to do some merging and start anew. Sounds good, but I notice that you have a bunch of fixup commits in here. Did you plan to rebase to squash those? Also, while I'm here, I think it makes sense to map Ctrl+F to `key_find` rather than `key_none` (though `key_none` can still exist to catch random modifier key combinations). I don't quite remember how the special keys work though so I'm not sure if that affects the XML-set key mapping.
NQNStudios commented 2025-03-29 13:17:28 +00:00 (Migrated from github.com)

Sounds good, but I notice that you have a bunch of fixup commits in here. Did you plan to rebase to squash those?

Yeah! If you merge the other two PRs I'll be rebasing this one onto master, and I'll autosquash when I do that.

Also, while I'm here, I think it makes sense to map Ctrl+F to key_find rather than key_none (though key_none can still exist to catch random modifier key combinations). I don't quite remember how the special keys work though so I'm not sure if that affects the XML-set key mapping.

I see there's not a key_find in eSpecKey. Would I add that? And I'm currently not sure where key combos get mapped to eSpecKeys although I can probably find it quickly (EDIT: yup). (If I already modified that part of the code in this PR I have forgotten. 😵‍💫 )

> Sounds good, but I notice that you have a bunch of fixup commits in here. Did you plan to rebase to squash those? Yeah! If you merge the other two PRs I'll be rebasing this one onto master, and I'll autosquash when I do that. > Also, while I'm here, I think it makes sense to map Ctrl+F to key_find rather than key_none (though key_none can still exist to catch random modifier key combinations). I don't quite remember how the special keys work though so I'm not sure if that affects the XML-set key mapping. I see there's not a key_find in eSpecKey. Would I add that? And I'm currently not sure where key combos get mapped to eSpecKeys although I can probably find it quickly (EDIT: yup). (If I already modified that part of the code in this PR I have forgotten. 😵‍💫 )
CelticMinstrel commented 2025-03-30 06:04:53 +00:00 (Migrated from github.com)

If you merge the other two PRs

I don't understand. There are no other PRs opened against master at the moment.

Certainly I could merge the tutorial and bolts PRs, but they are not against master so I don't understand why they would be treated as a blocker for this.

> If you merge the other two PRs I don't understand. There are no other PRs opened against master at the moment. Certainly I could merge the tutorial and bolts PRs, but they are not against master so I don't understand why they would be treated as a blocker for this.
NQNStudios commented 2025-03-30 16:31:03 +00:00 (Migrated from github.com)

Ok, yeah, I was thinking about things in a convoluted way that would have involved merging the tutorial PR then merging the tutorial branch to master then merging this PR to master. It can be way simpler.

I should autosquash this, then it can merge to master. Then the upstream tutorial branch should be synchronized with master. Then I rebase the tutorial PR without the cherry-picks which will be made unnecessary by merging this.

Ok, yeah, I was thinking about things in a convoluted way that would have involved merging the tutorial PR *then* merging the tutorial branch to master *then* merging this PR to master. It can be way simpler. I should autosquash this, then it can merge to master. Then the upstream tutorial branch should be synchronized with master. Then I rebase the tutorial PR without the cherry-picks which will be made unnecessary by merging this.
NQNStudios commented 2025-03-30 16:54:00 +00:00 (Migrated from github.com)

I've autosquashed the fixups.

The bolt PR onto scen_update might as well merge so it's one less thing to think about.

If you merge this and rebase the tutorial branch, let me know so I can rebase the tutorial PR. That one could be merged once rebased, but doesn't really need to be unless you are interested in starting designing the actual scenario before I might get to it.

I've autosquashed the fixups. The bolt PR onto scen_update might as well merge so it's one less thing to think about. If you merge this and rebase the tutorial branch, let me know so I can rebase the tutorial PR. That one could be merged once rebased, but doesn't really need to be unless you are interested in starting designing the actual scenario before I might get to it.
CelticMinstrel commented 2025-03-31 04:38:03 +00:00 (Migrated from github.com)

https://github.com/calref/cboe/pull/697#discussion_r2003960215

But not ones where it’s a choice between 1 and all, no? I can only think of
the “____ All” spells and there’s no selection dialog for those.

I think I completely lost track of this thread. I'm not aware of any cases where the player needs to choose between targeting one character or all characters, but the Affect category of special nodes are set to work on either one or all.

This is just a comment, not a request for changes or anything (it's a bit late for that now).

https://github.com/calref/cboe/pull/697#discussion_r2003960215 > But not ones where it’s a choice between 1 and all, no? I can only think of the “____ All” spells and there’s no selection dialog for those. I think I completely lost track of this thread. I'm not aware of any cases where the _player_ needs to choose between targeting one character or all characters, but the Affect category of special nodes are set to work on either one or all. This is just a comment, not a request for changes or anything (it's a bit late for that now).
CelticMinstrel commented 2025-04-05 04:37:51 +00:00 (Migrated from github.com)

Sorry for the delay on merging this. I'll try to get to it this weekend.

Sorry for the delay on merging this. I'll try to get to it this weekend.
NQNStudios commented 2025-04-05 19:26:33 +00:00 (Migrated from github.com)

No worries. I had to wrap up another project and I'm applying to jobs so this hasn't been a blocker or anything.

No worries. I had to wrap up another project and I'm applying to jobs so this hasn't been a blocker or anything.
Sign in to join this conversation.
No description provided.