[blueprint] Support new commandline syntax and semantics #1849

myk002 · 2021-05-05T20:26:29Z

This is the first stage of the blueprint plugin overhaul.

Depends on #1846 for support for negative depths and #1841 for the MapCache change (though the MapCache commits are included in this PR as well to avoid future merge conflicts)

The blueprint gui command has a soft dependency on DFHack/scripts#281. You'll be able to run the blueprint gui command, but it won't do anything until the gui/blueprint script has something in it.

User visible changes to blueprint:

depth and name parameters are now optional (defaulting to 1 and blueprint if not specified)
depth can now be negative to indicate that we should iterate through the levels top-down instead of the previous hard-coded bottom-up
new --cursor option for specifying start position (game cursor is not needed if this option is specified)

I tried to touch the original logic as little as possible, but I did add bounds checking for the input and clipping of the translation area at the map edge.

All docs are updated as well.

question:
I kept the lua functions that the blueprint plugin previously exported (dig(), place(), build(), and query()) though personally I'm not convinced they add any value over just running blueprint via run_command. In fact, the new implementations of these functions are just a wrapper around run_command. I don't see any callers in the repo, but I kept the functions for the sake of backward compatibility.

I should note, though, that I'm planning on changing the semantics of the phases in the future, including splitting the current query phase into config and rooms, so I don't think perfect backward compatibility will be feasible. Should I keep those functions or just remove them?

myk002 · 2021-05-08T14:44:59Z

note that unit tests will fail until #1846 is merged

- make depth and name parameters optional - allow depth to be negative to indicate top-down instead of the previous hard-coded bottom-up - add --cursor for specifying start position (game cursor is not needed if this param is specified)

(though "and" instead of "&&" seems to work in gcc!)

myk002 · 2021-05-11T04:13:19Z

note that unit tests will fail until #1846 is merged

latest develop merged. tests should pass now

lethosor

Looks good so far - I like the single path for command processing, and the use of Lua getopt. Still haven't played around with this in fortress mode, but from static review it looks good.

lethosor · 2021-05-12T04:10:13Z

plugins/blueprint.cpp

+
+    static struct_identity _identity;
+};
+static const struct_field_info blueprint_options_fields[] = {


This is an interesting approach that I felt like I should comment on. This looks good for type-safety! Long-term, I think we should generate type definitions like these with codegen somehow instead of manually, since there could be failures down the road if these field definitions aren't kept in sync with the structure definition. (using offsetof() to calculate offsets should help guard against most issues... but as a counterpoint, I'm not sure if the field definitions need to be ordered by offset, so rearranging fields could potentially have unintended consequences, for example).

Am I correct in assuming that you didn't define this in PluginStatics.cpp because the objects in question are short-lived and can't exist if the plugin is unloaded? (I was playing around with this and opts tended to end up at the same stack address every time!) I think this is safe, as long as you're sure that no Lua code retains references to opts (hopefully the garbage collector doesn't...).

In the interests of avoiding duplication, you could also consider a macro like FLD(), but the existing definition would admittedly be difficult to include.

+1 for generating the struct_field_info. As far as I could tell, this is the first time a marshal-able type has been manually defined in DFHack, so I guess the need for a generator just never came up before.

You are correct as to why I didn't put this in PluginStatics. The stack-allocated options struct doesn't live beyond the blueprint command execution so I figured I'd keep all the definitions together in the blueprint.cpp file instead of gaining a consistent identity object, but splitting the object definitions into PluginStatics.

Yeah, I manually expanded the #defines that are used in the generated struct_field_infos when I was figuring out how to do this. Not using the existing (but inaccessible) macros felt dirty, but copying the macros into blueprint.cpp felt somehow dirtier : p I didn't really want to get into refactoring DataStatics* in this PR, but that might be the way to go longer term.

lethosor

Looks great from a static review! I got sucked into details (e.g. documentation formatting) a bit, but those are minor things. Will try to test in more detail this weekend; let me know if you have any areas of concern in mind.

docs/Plugins.rst

plugins/blueprint.cpp

myk002 · 2021-05-15T06:19:51Z

Need to update the unit tests after deleting do_gui. I'll get that tomorrow

myk002 · 2021-05-15T13:00:24Z

Need to update the unit tests after deleting do_gui. I'll get that tomorrow

done

and factor out active cursor detection

lethosor · 2021-05-17T03:59:22Z

plugins/blueprint.cpp

@@ -755,16 +756,12 @@ command_result blueprint(color_ostream &out, vector<string> &parameters)
    DFCoord start(options.start);
    if (options.start.x == -30000)


Just noticed that df::coord has an isValid() method that does this too. I don't know how widely it's used - I know there are a lot of hardcoded checks for -30000 too - but if you end up making other changes, this could be something to use as well.

(This PR is still on my list. Hopefully tomorrow!)

lethosor

Looks really good! A couple minor things I noticed from testing; not sure if you've already fixed them elsewhere.

plugins/blueprint.cpp

plugins/lua/blueprint.lua

This was referenced May 5, 2021

[gui/blueprint] minimal logic for initial script DFHack/scripts#281

Merged

[gui/blueprint] add dummy gui/blueprint so plugin docs will build DFHack/scripts#282

Merged

myk002 force-pushed the myk_blueprint branch from 3758ffe to cb7079d Compare May 8, 2021 14:35

myk002 added 9 commits May 10, 2021 21:11

first stage of blueprint overhaul

a7a5a48

- make depth and name parameters optional - allow depth to be negative to indicate top-down instead of the previous hard-coded bottom-up - add --cursor for specifying start position (game cursor is not needed if this param is specified)

update blueprint docs

265f17a

update changelog

aba40b6

output args being passed to gui/blueprint

7c7d96b

fix formatting

0982955

fix typo in comment

a949065

add comment to parse_gui_commandline function

cc489db

add unit tests

816cd5c

yeah, this isn't lua

bee0c15

(though "and" instead of "&&" seems to work in gcc!)

myk002 force-pushed the myk_blueprint branch from cb7079d to bee0c15 Compare May 11, 2021 04:11

lethosor added this to In progress in 0.47.05-r2 via automation May 12, 2021

lethosor reviewed May 12, 2021

View reviewed changes

lethosor reviewed May 15, 2021

View reviewed changes

myk002 added 3 commits May 14, 2021 22:51

standardize formatting in docs

b0dba22

use setHotkeyCmd instead of dfhack.run_script

5a149f4

make short help text more generic

256ca13

remove unit tests for removed function

95d97b9

myk002 added 2 commits May 15, 2021 12:05

add getCursorCoords overload for df::coord

aff5c9b

and factor out active cursor detection

use new getCursorCoord call and move suspender up

07e29bd

lethosor reviewed May 17, 2021

View reviewed changes

lethosor reviewed May 21, 2021

View reviewed changes

plugins/blueprint.cpp Outdated Show resolved Hide resolved

plugins/lua/blueprint.lua Outdated Show resolved Hide resolved

myk002 added 2 commits May 21, 2021 06:34

modify start, not options.start when getting coord

0409b7b

error on invalid phase names

1aaed3a

lethosor approved these changes May 25, 2021

View reviewed changes

lethosor merged commit 4f976a5 into DFHack:develop May 25, 2021

0.47.05-r2 automation moved this from In progress to Done May 25, 2021

lethosor mentioned this pull request Jun 30, 2021

[tiletypes] add --cursor and --quiet options to support scripted invocation #1867

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[blueprint] Support new commandline syntax and semantics #1849

[blueprint] Support new commandline syntax and semantics #1849

myk002 commented May 5, 2021 •

edited

myk002 commented May 8, 2021

myk002 commented May 11, 2021

lethosor left a comment

lethosor May 12, 2021

myk002 May 12, 2021

lethosor left a comment

myk002 commented May 15, 2021

myk002 commented May 15, 2021

lethosor May 17, 2021

lethosor left a comment

		@@ -755,16 +756,12 @@ command_result blueprint(color_ostream &out, vector<string> &parameters)
		DFCoord start(options.start);
		if (options.start.x == -30000)

[blueprint] Support new commandline syntax and semantics #1849

[blueprint] Support new commandline syntax and semantics #1849

Conversation

myk002 commented May 5, 2021 • edited

myk002 commented May 8, 2021

myk002 commented May 11, 2021

lethosor left a comment

Choose a reason for hiding this comment

lethosor May 12, 2021

Choose a reason for hiding this comment

myk002 May 12, 2021

Choose a reason for hiding this comment

lethosor left a comment

Choose a reason for hiding this comment

myk002 commented May 15, 2021

myk002 commented May 15, 2021

lethosor May 17, 2021

Choose a reason for hiding this comment

lethosor left a comment

Choose a reason for hiding this comment

myk002 commented May 5, 2021 •

edited