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

Update the special node edit dialog with a bunch of new pickers #655

Merged
merged 31 commits into from
Mar 3, 2025

Conversation

CelticMinstrel
Copy link
Member

The primary goal of this pull request is to add a Choose button (or similar) to as many fields of the Special Node edit dialog as possible. It also reworks the format of the picker dictionary (as per #607) in order to more easily expand its capabilities.

@NQNStudios
Copy link
Collaborator

I'm really excited about this one for when I get to actually make a scenario.

@NQNStudios
Copy link
Collaborator

Today I noticed that damage type pickers use boom graphics in the picker, but some booms appear multiple times, I think one didn't have a boom and it didn't really make perfect sense. Maybe worth looking at if you haven't.

@CelticMinstrel
Copy link
Member Author

CelticMinstrel commented Feb 23, 2025

Today I noticed that damage type pickers use boom graphics in the picker, but some booms appear multiple times, I think one didn't have a boom and it didn't really make perfect sense. Maybe worth looking at if you haven't.

The damage type picker is one of the ones that uses an "augmented" graphic picker. When you select one of the options, a string appears at the bottom left saying what it is. By doing this in the damage picker, you can see that one of the "magic" booms is magic and the other is unblockable. Similarly, the extra two "blood" booms are demon and undead damage.

I can't deny that there could be a better way of doing it, but it functions.

@NQNStudios NQNStudios added enhancement scenedit Affects the scenario editor game Affects the game, as opposed to the editors labels Feb 23, 2025
@CelticMinstrel
Copy link
Member Author

I think the last big thing I plan to add here is an SDF picker. It would sort of combine aspects of the location picker and the editable string picker.

@CelticMinstrel CelticMinstrel marked this pull request as ready for review March 2, 2025 05:43
@CelticMinstrel
Copy link
Member Author

CelticMinstrel commented Mar 2, 2025

I'm marking this ready for review. I do still want to add a few more things:

  • A system to enable special node pickers to depend on the values of other fields.

  • Fix all cases where the special node field prompt overflows the available space, and cases where the prompt is the field name when it should be "Unused".

  • Add help strings for every special node.

  • Either fix the If Statistic? picker so you can select things like Current HP, or add a new node that allows that.

  • Most things in the dialogue node editor don't have proper pickers – fix that.

  • Use the SDF picker in many more places.

  • Remove Edit Saved Item Rectangles. Add a toolbar button instead. (This is instead of making Edit Saved Item Rectangles use the location picker.)

  • Add minor variations of the location picker for rectangles, paths, or other cases where multiple locations are needed.

  • Add a picker for the strings in the Story Dialog special node.

  • Add pickers for pointers. Details still TBD.

  • Something to do with spell patterns TBD.

But they don't necessarily have to be in this PR. Whether they are or not depends on when they get done. I'll most likely continue to work on this branch while the PR is reviewed, so whatever gets done in that time will probably be pushed.

@CelticMinstrel CelticMinstrel removed the game Affects the game, as opposed to the editors label Mar 2, 2025
Basically what that means is that the magic characters are only parsed at startup – the scenario editor code doesn't need to understand them, instead using an enum.

This is part of the work for #607
Though this creates the framework, the actual help strings have not been filled in yet.
These are mostly errors that were already wrong in the old dictionary.
It's possible that some are things missed in the refactor though.
…nnotate those into the button dictionary.

This commit doesn't implement any new pickers, only adds the data for later use.
Use it for the MSG_PAIR picker and the PIC_NONE picker.
This also adds partial support for a custom vehicle graphic.
@CelticMinstrel CelticMinstrel force-pushed the tilemap_and_stuff branch 2 times, most recently from 48003fc to e976e9a Compare March 2, 2025 15:35
Copy link
Collaborator

@NQNStudios NQNStudios left a comment

Choose a reason for hiding this comment

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

The scope of the changes is huge and I think this is taking the scenario editor in a very good direction overall. I still need to look at the actual refactor of the node dictionary.

if(type == "name") {
vehicle->GetText(&list[i].name);
} else if(type == "pic") {
vehicle->GetText(&list[i].pic);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this addressing #299? Do existing scenarios need to be updated to specify vehicle graphics / is there a way to edit a vehicle's graphic in the editor yet, or is that another thing for the TODO list?

Copy link
Member Author

Choose a reason for hiding this comment

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

It only adds the storage space for that information in the scenario format – it's not yet used for anything. So yes, it's related to #299.

virtual ~cControl();
/// Draw the control into its parent window.
virtual void draw() = 0;
cControl& operator=(cControl& other) = delete;
cControl(cControl& other) = delete;
/// Sets the positioning method of this control.
/// Note: this only has an effect when called from another control's parse function.
void setPositioning(const std::string& anchor_id, ePosition h, ePosition v) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any way this could break/change existing dialogs with relative positioning?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so? First of all, nothing could be broken unless this is actually called. Currently, it's only called from the new tilemap widget. I don't think the usage in the tilemap widget can break anything. The tilemap widget fiddles with the names of its nested elements, so essentially all it does is correct the anchor of the nested elements to refer to the adjusted name instead of the name from the XML.

<led name='inbank' top='285' left='330'>Include in a job bank:</led>
<field name='bank1' top='306' left='344' width='110' height='16'/>
<field name='bank2' top='332' left='344' width='110' height='16'/>
<led name='rel' wrap='true' top='208' left='345'>Deadline is relative to start day</led>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could either mean the scenario start day, or quest start day. I'm not sure which it means.

@@ -185,10 +196,11 @@ Unused
0 - allow, 1 - prevent
Unused
Unused
0 - don't force, 1 - force if blocked
Force allow if blocked?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Kind of confused on the meaning here.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's something like "if the other field is set to 'allow', override the terrain's blockage". I think it only applies when the action being prevented is movement, but not quite sure.

Better suggestions for the wording are welcome.

@@ -55,25 +55,25 @@ void cStack::draw() {
for(auto& p : controls) {
p.second->draw();
}
if(drawFramed) drawFrame(2, frameStyle);
drawFrame(2, frameStyle);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you figure out the bug you mentioned with the white lines appearing?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did not. I strongly suspect that this is the cause but I didn't get around to investigating in detail.

@NQNStudios
Copy link
Collaborator

When an LED is placed next to ex2a, if the value in the field is -1, it should change to 0, right? Currently you have to click it once to turn it on (1) and again to turn it off (0). Maybe the code already handles -1 as false for those bool flags, but it'd put me more at ease if the value snapped to a known valid one.

@NQNStudios
Copy link
Collaborator

Screenshot 2025-03-02 at 3 15 22 PM

Some special nodes are showing as "error" in the One-Time category.

@NQNStudios
Copy link
Collaborator

When you scroll away from your selected SDF in the SDF picker, then scroll back returning it into view, it makes a sound when that LED becomes selected again. Ideally it should change state silently because it wasn't clicked again.

@NQNStudios
Copy link
Collaborator

I think the SDF picker button is kind of missable where it is.

Screenshot 2025-03-02 at 3 40 34 PM

If the framed labels were narrowed you could fit it as a regular size button with a label on it, to the left of the Part A label. The Part A label is already wider than the Part B one, dunno if there's a reason for that.

@CelticMinstrel
Copy link
Member Author

When an LED is placed next to ex2a, if the value in the field is -1, it should change to 0, right? Currently you have to click it once to turn it on (1) and again to turn it off (0). Maybe the code already handles -1 as false for those bool flags, but it'd put me more at ease if the value snapped to a known valid one.

I checked a lot of them in boe.specials.cpp – most of them just do a "greater than zero" check to delineate true from false. It might make sense to warp it to 0, but I decided that was a bit beyond the scope of what I was doing. It's probably something that would make sense for other things besides just toggle… though I'm not convinced it's a good idea in general, as it makes "selecting a different type" perform a possibly lossy change.

Some special nodes are showing as "error" in the One-Time category.

Yes. They were originally "Unused", but I'd need to add an explicit node button dictionary entry to preserve that. I think in practice the solution is to squish them down so that there isn't a gap.

I'm pretty sure changing the internal number of a special node won't actually affect anything at all (feel free to correct me if I'm wrong) – the file references them by opcode name, and everything in the code uses the enum. As for legacy importing, it uses the original integer as the source node and the enum for the target, so it too wouldn't be affected. All I need to do is make sure that the squish happens in both the enum and the file with the list of all the opcode names, and it should be fine.

The other possibility is explicitly declaring them in the button dictionary. That would make them display as "Unused" like they used to.

When you scroll away from your selected SDF in the SDF picker, then scroll back returning it into view, it makes a sound when that LED becomes selected again. Ideally it should change state silently because it wasn't clicked again.

Agreed. I'll see what I can do about that. Do you get the same in the editable string picker or is it only the SDF picker that does this?

I think the SDF picker button is kind of missable where it is.

Well, this isn't new (it's already the case for rectangle nodes that already had pickers before I did this).

…t each of the strings inline and even add new ones.

This just implements the guts of the dialog, without using it for anything yet.

It also fixes a bug that caused a blank page to appear in the string picker if the total number of strings was an exact multiple of 40.

Closes #563
…cker.

It's used not only in special node editing (for Event Happens and If Event Happened?)
but also in the Townperson Advanced, Talk Node, and Quest editors.
…ring picker.

Used in special node editing and also item editing.
These are all cases where similar items stack but these ones for some reason didn't.

Gemstones/Ores: Ruby, Crystal, Rough Diamond
Powders: Dust of Hiding and Choking
Missiles: Poison Darts; Arrows of Life and Light; Burning and Exploding Arrows
Reagents: Asp Fangs
…rid.

Use it for the Edit Terrain Object dialog.
…ector.

It's currently used in special node editing and in advanced town details.
All controls now store a reference to their direct parent,
whether it be the dialog itself or a container control.

Every dialog control now has a guaranteed parent, which abstracts away
the three possible types of parents (dialog, container, and plain window).

The control name is now stored in the control from the moment it is parsed from the XML.
This means that it's set before the parseContent function, though after parseAttribute.
…he screen

Note: This could make certain larger dialogs always crash with a high enough pixel scale. Needs testing.
…ng it a name.

In effect, this is a combination of two of the previous pickers:
the location picker, and the editable string picker.

This required quite a significant rework of how the tilemap places its children.

Currently it's only used in special node editing.
I plan to add its use in many other places too though.
@CelticMinstrel CelticMinstrel merged commit 3d48cb1 into master Mar 3, 2025
10 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement scenedit Affects the scenario editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants