2.0 Beta 1 Pt 2 #697
Reference in New Issue
Block a user
No description provided.
Delete Branch "2.0b1-b"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
@@ -941,0 +996,4 @@break;}const cInvenSlot& pick_slot = univ.party[i].has_abil_equip(eItemAbil::LOCKPICKS);if(!pick_slot){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.
@@ -8,34 +8,15 @@I think I like using
std::tiehere.@@ -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);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:
Then the more transient ones on top:
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.
@@ -1039,16 +1041,22 @@ short scan_for_response(const char *str) {return -1;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.
@@ -1039,16 +1041,22 @@ short scan_for_response(const char *str) {return -1;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.
@@ -941,0 +996,4 @@break;}const cInvenSlot& pick_slot = univ.party[i].has_abil_equip(eItemAbil::LOCKPICKS);if(!pick_slot){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.
@@ -1039,16 +1041,22 @@ short scan_for_response(const char *str) {return -1;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.)
@@ -1039,16 +1041,22 @@ short scan_for_response(const char *str) {return -1;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.
@@ -1039,16 +1041,22 @@ short scan_for_response(const char *str) {return -1;Or we could add something to the spell info struct for this.
@@ -1039,16 +1041,22 @@ short scan_for_response(const char *str) {return -1;@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.
@@ -941,0 +996,4 @@break;}const cInvenSlot& pick_slot = univ.party[i].has_abil_equip(eItemAbil::LOCKPICKS);if(!pick_slot){@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.
@@ -941,0 +996,4 @@break;}const cInvenSlot& pick_slot = univ.party[i].has_abil_equip(eItemAbil::LOCKPICKS);if(!pick_slot){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.
@@ -1039,16 +1041,22 @@ short scan_for_response(const char *str) {return -1;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.
(sent via email, moved to proper thread)
@@ -941,0 +996,4 @@break;}const cInvenSlot& pick_slot = univ.party[i].has_abil_equip(eItemAbil::LOCKPICKS);if(!pick_slot){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.
@@ -37,7 +37,24 @@ void reset_item_max();short item_val(cItem item);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?@@ -37,7 +37,24 @@ void reset_item_max();short item_val(cItem item);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.
@@ -12,10 +12,14 @@#include <algorithm>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
defClror similar somewhere that handles this.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_strandloop_to_strto just contain the whole sentence? Then this would becomeoutput.setText(loop_from_str + " " + loop_to_str").@@ -12,10 +12,14 @@#include <algorithm>Is it this, in message.cpp?
@@ -12,10 +12,14 @@#include <algorithm>oh
parent.getDefTextClr()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.
@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.
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.
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_findrather thankey_none(thoughkey_nonecan 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.Yeah! If you merge the other two PRs I'll be rebasing this one onto master, and I'll autosquash when I do that.
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. 😵💫 )
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.
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.
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.
https://github.com/calref/cboe/pull/697#discussion_r2003960215
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).
Sorry for the delay on merging this. I'll try to get to it this weekend.
No worries. I had to wrap up another project and I'm applying to jobs so this hasn't been a blocker or anything.