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

Add various tools, and update gui/gm-unit #108

Merged
merged 18 commits into from
Jan 19, 2020
Merged

Add various tools, and update gui/gm-unit #108

merged 18 commits into from
Jan 19, 2020

Conversation

Atkana
Copy link
Contributor

@Atkana Atkana commented Oct 31, 2019

I recently got through updating a bunch of my modtools + old edits to gui/gm-unit and thought they might be worth submitting. Only now have I noticed that some of them are similar to those submitted in another pull.

modtools/set-need

Provides command line and script function options for editing a unit's needs.

modtools/set-belief

Provides command line and script function options for editing a unit's values. I used the term "belief" instead of the game's usage of "value" because of the latter's use as a generic programming term (which might cause some confusion as to what the script does). The randomisation function doesn't follow the proper base game method (I don't know how it works), but it's good enough for a start - anyone who knows how it works is welcome to implement it / tell me.

modtools/set-personality

Provides command line and script function options for editing a unit's personality facets. Same case for randomisation as modtools/set-belief.

modtools/pref-edit

Provides command line and script function options for editing a unit's preferences.

set-orientation

Provides command line and script function options for editing a unit's sexual orientation.

Edits to gui/gm-unit

Added: Personality editor, belief editor, orientation editor, Body appearance editor, colors editor. Originally, the latter two were supposed to be separate scripts/modtools, but I couldn't decide how I wanted to make it, so for now those two functions are hardcoded into that editor. In the future, I might still make them and make a new edit to separate it all out.

Also sorry if I screwed up updating my master fork, I barely know how to use github :P

Copy link
Member

@warmist warmist left a comment

Choose a reason for hiding this comment

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

Very nice. A lot of work to make everything work!

The PrayOrMedidate bug needs to merged first or you can't exit the screen (devel/pop-screen helps).

Add, remove, or edit the preferences of a unit.
Requires a modifier, a unit argument, and filters.

:unit <UNIT ID>:
Copy link
Member

Choose a reason for hiding this comment

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

Afaik [] is used for optional arguments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked the style guide but couldn't find anything about using square brackets (though did find I'm not sticking to them exactly, oops :P). It's kinda confusing as to whether the unit id is an optional addition because of how the scripts handle missing/invalid targets.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's probably something we could put in our documentation (I'll try to find a good place for it)

Copy link
Member

Choose a reason for hiding this comment

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

It's more of a "unix" convention or just general commandline tool convention.

gui/gm-unit.lua Outdated
local setbelief = dfhack.reqscript("modtools/set-belief")
local setpersonality = dfhack.reqscript("modtools/set-personality")
local setneed = dfhack.reqscript("modtools/set-need")
local setorientation = dfhack.reqscript("set-orientation")
Copy link
Member

Choose a reason for hiding this comment

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

any reason this is not same as others? (i.e. modtools folder)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't include set-orientation as a modtool basically as a whim (and I don't fully know what counts as a modtool and what doesn't). If it seems like it should belong as a modtool (or that the others shouldn't), somebody more knowledgable about them is welcome to change them.

Copy link
Member

@lethosor lethosor Jan 18, 2020

Choose a reason for hiding this comment

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

Generally if they are end-user-facing they shouldn't be in modtools (my rule of thumb is "end users shouldn't have to type 'modtools' into the DFHack console"). Looks like most of these are at least somewhat user-facing, so I would probably move them out.

Comment on lines +846 to +847
self.target_unit = args.target_unit
self.partChoice = args.partChoice
Copy link
Member

Choose a reason for hiding this comment

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

In cases like this, you can use class.ATTRS={name=default} with DEFAULT_NIL if you want it to be nil by default. That way if you pass something to args it gets auto-set to a value. More about it here: https://dfhack.readthedocs.io/en/stable/docs/Lua%20API.html#class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahah, so that's how that works. I'll remember this for the future!

@Atkana Atkana requested a review from warmist November 4, 2019 07:39
@Atkana
Copy link
Contributor Author

Atkana commented Nov 4, 2019

Is there something I need to do to get rid of the red x?

@warmist
Copy link
Member

warmist commented Nov 4, 2019

According to the build log set-orientation.lua:24 there is a unexpected intendation and there are some trailing whitespaces in set-need.lua:554 and gm-unit.lua: lines 679, 988, 1138, 1240.

@Atkana
Copy link
Contributor Author

Atkana commented Nov 4, 2019

I thought lua didn't care about whitespace, how come it's a problem? Also the error isn't really helpful to me because I don't know what expected / unexpected indentation is. I've edited it to match as closely to all the other scripts I've submitted but it still isn't working, so I have no idea why that one in particular is causing a problem.

@warmist
Copy link
Member

warmist commented Nov 4, 2019

AFAIK first one is documentation generation warning.
Second ones are our whitespace requirements. I'm not sure why project have these requirements but usually it's for tidyness and cleaner commits

@lethosor
Copy link
Member

lethosor commented Nov 4, 2019

Yeah, the current error is from Sphinx (parsing the documentation as RST). I agree, I don't see what's wrong with it, but I'll look into it.

Warning, treated as error:
../../../dfhack/scripts/modtools/pref-edit.lua:25: ERROR: Unexpected indentation.

@@ -0,0 +1,460 @@
-- Changes the beliefs (values) of units.
Copy link
Member

Choose a reason for hiding this comment

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

Just realized this sounds similar to assign-belief from #105 - haven't really looked into how similar they are, but we might need to consolidate them.
None of your other scripts sound as similar, but there could be some overlap too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hehe yeah, that's the one I was alluding to :P

Copy link
Member

Choose a reason for hiding this comment

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

Since neither are merged yet, I'm thinking I'll take the ones from #105 and make sure any extra features from these make it in too - sound ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you want, but you'd be having to add quite a lot of features. It's awkward that they both have overlap and dependencies on themselves. It might be dumb but you could always include both, it's not like the regular scripts aren't without redundancies (or at least some will be with the inclusion of either of these) :P

Copy link
Member

Choose a reason for hiding this comment

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

The gm-unit features are neat, and I don't feel like reimplementing those using the other scripts, so I'll merge this as-is and we can refactor it later.

Copy link
Member

@lethosor lethosor left a comment

Choose a reason for hiding this comment

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

FYI, I had to make some changes in the docs to get them to build without errors. Notably, * and \ are special characters in RST, and need to be escaped or put in code blocks (I opted for the latter). Your \-1 was showing up as "-1", and the \s weren't showing up at all.

image

@lethosor
Copy link
Member

Another update: looks like the addition of NONE values of value_type and personality_facet_type in DFHack/df-structures#331 broke set-belief and set-personality, respectively.

@lethosor lethosor merged commit dc37785 into DFHack:master Jan 19, 2020
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.

3 participants