Rework special node button dictionary #607

New Issue

2025-02-15T07:08:44Z

CelticMinstrel commented

2025-02-15 07:08:44 +00:00

(Migrated from github.com)

The special node button_dict (in special.cpp) encodes what sort of button should be placed on each special node field and what kind of picker should appear when that button is pressed. It's encoded as 2-dimensional arrays with each ASCII character having a different meaning. It's quite compact and works just fine, but actually editing it to fix issues or add a new type is rather painstaking.

I'm trying to think of a better way to do it that's easier to edit while not being overly verbose. This is in service of eventually adding several new picker types (including some dynamic pickers), as well as a "simple toggle" case which replaces the entire field with an LED.

Ultimately what it needs to do is to populate a node_properties_t struct for each possible special node type. Directly declaring the array of node_properties_t would probably be possible but feels like it would be overly verbose.

The special node `button_dict` (in special.cpp) encodes what sort of button should be placed on each special node field and what kind of picker should appear when that button is pressed. It's encoded as 2-dimensional arrays with each ASCII character having a different meaning. It's quite compact and works just fine, but actually editing it to fix issues or add a new type is rather painstaking. I'm trying to think of a better way to do it that's easier to edit while not being overly verbose. This is in service of eventually adding several new picker types (including some dynamic pickers), as well as a "simple toggle" case which replaces the entire field with an LED. Ultimately what it needs to do is to populate a `node_properties_t` struct for each possible special node type. Directly declaring the array of `node_properties_t` would probably be possible but feels like it would be overly verbose.

NQNStudios commented

2025-02-15 14:01:10 +00:00

(Migrated from github.com)

Oh dear... Somehow I'd never seen this before now.

e83ad32618/src/scenario/special.cpp (L583)

My gut reaction is that I hate it.

Oh dear... Somehow I'd never seen this before now. https://github.com/calref/cboe/blob/e83ad326188a3cd13ed6a524efcc4c4013d1be9b/src/scenario/special.cpp#L583 My gut reaction is that I hate it.

NQNStudios commented

2025-02-15 14:11:22 +00:00

(Migrated from github.com)

What if the information was encoded as XML, which was written by a WYSIWYG editor showing what the edit special node dialog would look like when the node's definition was parsed?

CelticMinstrel commented

2025-02-15 16:06:21 +00:00

(Migrated from github.com)

That sounds like extreme overkill for something that's only going to be edited by developers… it's not the most terrible of ideas, but I don't think I like it.

CelticMinstrel commented

2025-02-16 18:20:59 +00:00

(Migrated from github.com)

I got started on trying to clean this up. The above commit defines the structure of how the information should be in memory, while retaining the magic characters during loading.

Any comments are welcome.

I got started on trying to clean this up. The above commit defines the structure of how the information should be in memory, while retaining the magic characters during loading. Any comments are welcome.

NQNStudios commented

2025-02-18 21:03:02 +00:00

(Migrated from github.com)

I was baffled recently by trying to parse through this area of code the other day:

3bd4adcf6a (diff-176a6d3d8f719743296a0862725f49ddc4706bde78837e720bec1ab78b9c6200L880)

And I can say that it is a lot more readable with enums than with magic characters as the case labels.

I was baffled recently by trying to parse through this area of code the other day: https://github.com/calref/cboe/commit/3bd4adcf6a7b8b4c706b3d8d72592a8144c532eb#diff-176a6d3d8f719743296a0862725f49ddc4706bde78837e720bec1ab78b9c6200L880 And I can say that it is a lot more readable with enums than with magic characters as the case labels.

CelticMinstrel commented

2025-02-20 01:23:29 +00:00

(Migrated from github.com)

Does it seem like the above WIP commit might be going in the right direction? (Pay the closest attention to the newly-added file.)

NQNStudios commented

2025-02-20 01:38:00 +00:00

(Migrated from github.com)

Seems like you accidentally deleted the weight field from item.hpp?

But I really like the way it's going, with the builder pattern. It's much more readable to see which node has which properties.

(The only thing about the builder pattern is, it's only readable if you recognize it & know how it works. When I first saw spells.cpp I was super confused about what it was representing. So, we should make sure to have comments explaining well what the file is doing and how to add new nodes.)

Seems like you accidentally deleted the weight field from item.hpp? But I really like the way it's going, with the builder pattern. It's much more readable to see which node has which properties. (The only thing about the builder pattern is, it's only readable if you recognize it & know how it works. When I first saw `spells.cpp` I was super confused about what it was representing. So, we should make sure to have comments explaining well what the file is doing and how to add new nodes.)

CelticMinstrel commented

2025-02-20 06:26:36 +00:00

(Migrated from github.com)

Considering various ways of formatting this. The above commit shows "compact" style in specials-general and specials-oneshot, but "verbose" style in specials-affect.

Beyond the difference shown there, there are a few other options:

I could make .finish() be implicit if I didn't use auto on the declarations (they'd need to be declared as node_property_t).
I could also avoid auto but still keep .finish().
Even if not using the "verbose" style, I'd probably break up the longest ones into multiple lines, but not one field per line.

There's no need to respond to this, but if you have any thoughts on the matter at all, feel free to let me know.

Considering various ways of formatting this. The above commit shows "compact" style in specials-general and specials-oneshot, but "verbose" style in specials-affect. Beyond the difference shown there, there are a few other options: * I could make `.finish()` be implicit if I didn't use `auto` on the declarations (they'd need to be declared as `node_property_t`). * I could also avoid `auto` but still keep `.finish()`. * Even if not using the "verbose" style, I'd probably break up the longest ones into multiple lines, but not one field per line. There's no need to respond to this, but if you have any thoughts on the matter at all, feel free to let me know.

NQNStudios commented

2025-02-20 17:30:06 +00:00

(Migrated from github.com)

I don't think I mind the verbose style. I do think finish() should be implicit.

I don't think I mind the verbose style. I do think `finish()` should be implicit.

CelticMinstrel commented

2025-02-22 02:49:39 +00:00

(Migrated from github.com)

I think that completes the rework of the button dictionary? It looks like the pickers are working correctly still.

NQNStudios commented

2025-02-22 03:19:24 +00:00

(Migrated from github.com)

It can't really break the game, can it? Just the editor?

It can't really break the *game*, can it? Just the editor?

CelticMinstrel commented

2025-02-22 03:28:25 +00:00

(Migrated from github.com)

It can actually break the game, since this structure is also used by the function that parses special nodes from a file.

It _can_ actually break the game, since this structure is also used by the function that parses special nodes from a file.

CelticMinstrel commented

2025-03-03 02:05:31 +00:00

(Migrated from github.com)

This is closed by #655 / 4174ccd470

This is closed by #655 / 4174ccd470ce83d8072ba376a929060f6d824146

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: openboe/oboe#607