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

[quickfort] allow blueprints to specify bins/barrels/wheelbarrows for stockpiles #254

Merged
merged 1 commit into from
Feb 20, 2021

Conversation

myk002
Copy link
Member

@myk002 myk002 commented Feb 1, 2021

in #place blueprints.

syntax documentation in DFHack/dfhack#1768

fields.max_wheelbarrows = ntiles
if max_wb < 0 then max_wb = 1 end
if max_wb >= ntiles - 1 then
fields.max_wheelbarrows = ntiles - 1
Copy link
Member

Choose a reason for hiding this comment

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

I think this is the case, but just to confirm - you are directly setting this field, not sending keypresses to the game, right? Asking because tweak max-wheelbarrows changes the in-game wheelbarrow UI (the vanilla UI uses w to cycle through numbers and only goes up to 3), and it's not always obvious that this is a DFHack feature.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's right - the variable ui for setting wheelbarrows is the whole reason for this feature. This will set the wheelbarrow (and/or container) count programmatically

@lethosor
Copy link
Member

How should something like this be handled?

#place
`,`,`,`
`,f2,f,`
`,f,`,`
`,`,`,`

Currently the f2 cell produces a separate 1-tile stockpile with 1 barrel allowed, and the other two cells are connected to form a 2-tile, 2-barrel stockpile. Given that removing the 2 produces a single stockpile, I feel like the container count should be ignored when determining whether stockpiles are unique. However, that would lead to ambiguity in cases like this:

#place
`,`,`,`
`,f2,f3,`
`,f,`,`
`,`,`,`

so I'm not really sure what makes sense here.

@myk002
Copy link
Member Author

myk002 commented Feb 19, 2021

Good point. So the options are:

  1. Status quo, require the same number across all tiles of stockpiles. Pros: fits with current expectations for boundary detection, no confusion about whether adjacent tiles are part of the same stockpile (same text == same stockpile), no code changes. Cons: extra text in blueprint that doesn't really add value, just clutter. Changing the number of containers assigned to a stockpile requires changing text in every tile of that stockpile.

  2. Ignore numbers for the purposes of boundary detection. Pros: only add container numbers to a single time, making it easy to change and simplifying the text in all the other stockpile tiles. Cons: potential for confusion, both in visually determining boundaries and in the case you brought up where two tiles have conflicting numbers, also need to figure out how to implement (will need to keep aux metadata about the region. Text == type will no longer be enough).

Not obvious to me which route is better. Ease of editing and maintenance, less clutter vs visual clarity of meaning.

The same question will come up when I implement stockpile labels, which will allow disjoint sections to be considered part of the same stockpile. The problem gets even worse then because labels will be much longer than the numbers added here. The decision we make now will decide the implementation of that feature too.

I'm leaning towards ignoring the numbers for purposes of boundary detection, mostly for its decluttering property, which will become even more important with labels.

How to handle conflicting specifications, though? I'll come back and finish this thought in a bit.

@myk002
Copy link
Member Author

myk002 commented Feb 19, 2021

I should note that I expect most stockpile declarations to be in expansion syntax: f(4x5), so while the decisions we make here will be far-reaching, whatever we go with is unlikely to affect the majority of users.

@myk002
Copy link
Member Author

myk002 commented Feb 19, 2021

Ok, conflicting specifications. Again, choices here will propagate to the future label feature.

Option 1, log but ignore conflicts, just use one. Option 2, fail with parse error on conflict detection. Option 3, consider conflict as defining a separate stockpile.

I'm thinking option 3 is most flexible. For each stockpile, each tile can have either no metadata or the same metadata as was declared elsewhere in the same stockpile. This allows the convenient method of only defining the metadata once, i.e.:

#place
f3,f,f
f,f
f

But also allows two adjacent, non-expansion syntax stockpiles to be differentiated:

#place
f3,f3,f3
f3,f3,f2
f3,f2,f2

Labels could make this a little more clear:

#place
f3:labelA,f:labelA,f:labelA
f:labelA,f:labelA,f2:labelB
f:labelA,f:labelB,f:labelB

And of course labels would be able to differentiate the piles even if they were both f2. Ambiguous layouts would result in a parse error:

#place should have parse error
f3,f,f2
f3,f,f2

I think this will give the blueprint designer simple convenience in the more common case but will also provide the flexibility required for complex layouts. Thoughts?

@myk002
Copy link
Member Author

myk002 commented Feb 19, 2021

I've looked into the required code changes. It alters a basic assumption the code has had up to this point -- that the literal cell text is the boundary map key. This assumption goes deep enough that I think the planned changes are too risky to do without regression tests, especially so close to a major release.

Since the proposed changes will be backwards compatible with the current implementation, how about if we merge this as-is, then I spend some time setting up automated regression tests, then make the changes after I'm more certain things won't break?

edit: I updated the docs in DFHack/dfhack#1768 to explain how to specify container counts in stockpiles that do not use expansion syntax.

@lethosor
Copy link
Member

I figured it would be complicated to make every possibility work well. Thanks for looking into it! I think DFHack/dfhack@3278519 is plenty good enough for now. I also don't expect stockpiles specified with non-expansion syntax and with custom container counts to be very common, so glad to hear that you agree.

@lethosor lethosor merged commit b834623 into DFHack:master Feb 20, 2021
@myk002 myk002 deleted the myk_stockpile_containers branch February 20, 2021 07:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants