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

WIP New lua script: a gui pregnancy tool #873

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Pebob
Copy link
Contributor

@Pebob Pebob commented Oct 23, 2023

TODO:
1.Add a help string.
2.The exit function also isnt working as intended right now.
3.changelog.txt would also need to be update

anything else?

Would appreciate any feedback (new to lua / dfhack scripts)

@Pebob Pebob changed the title New lua script: a gui pregnancy tool WIP New lua script: a gui pregnancy tool Oct 23, 2023
Copy link
Member

@myk002 myk002 left a comment

Choose a reason for hiding this comment

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

Looks pretty good! Thank you for writing this! What about the case of creating a pregnancy without a specified father? You already have the logic for duplicating the female genes in the case of a historical father. You could extend this to support creating a pregnancy without a selected father at all.

Copy link
Member

Choose a reason for hiding this comment

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

docs for this script would go in docs/gui/pregnancy.rst -- see other files in that directory and copy the format.

Copy link
Member

Choose a reason for hiding this comment

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

docs are still required

gui/pregnancy.lua Outdated Show resolved Hide resolved
gui/pregnancy.lua Outdated Show resolved Hide resolved
gui/pregnancy.lua Outdated Show resolved Hide resolved
gui/pregnancy.lua Outdated Show resolved Hide resolved
gui/pregnancy.lua Outdated Show resolved Hide resolved
Comment on lines 246 to 247
local count = #self.msg
for i=0, count do self.msg[i]=nil end --empty self.msg
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
local count = #self.msg
for i=0, count do self.msg[i]=nil end --empty self.msg
self.msg = {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... i tried this but only seem to get empty strings in the label widgets...

gui/pregnancy.lua Outdated Show resolved Hide resolved
end
-- self.success = false
if bypass or force then
--TODO add GUI to select the number of months for pregnancy timer
Copy link
Member

Choose a reason for hiding this comment

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

You can use a RangeSlider for this. See example here: https://github.com/DFHack/dfhack/blob/develop/plugins/lua/zone.lua#L169-L217

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I got it working (kinda) i had to reverse index my values and use the index for the get_left_idx_fn and get_right_idx_fn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Re the RangeSlider - I have some issues dragging the slider to the minimum value (for me I have to slide all the way to the right to select the minimum).
Its not the biggest deal.

I did have to make the window pretty wide (initially i was planning to give the options in weeks). I guess RangeSlider is most suited for when there are <10 or so options?

I am curious if the reverse indexing was correct too.

gui/pregnancy.lua Show resolved Hide resolved
@Pebob
Copy link
Contributor Author

Pebob commented Oct 28, 2023

Yeah, I was planning on adding a option to not have a father (you can select the mother as the father already, but it would be better to do it more explicitly (and not populate the relationship_ids.Father)

I did do some searching but I couldnt find the genes or historical figures (i found their attributes etc). Do the genes exist? or do you (or anyone else) know if there are base genes for a entity?

Thank you very much for the detailed feedback.

added Newline and self.msg changes to pregnancy.lua
Copy link
Member

Choose a reason for hiding this comment

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

docs are still required

gui/pregnancy.lua Show resolved Hide resolved
frame_title='My Window',
frame={w=50, h=45},
frame_title='Pregnancy manager',
frame={w=64, h=35},
resizable=true, -- if resizing makes sense for your dialog
resize_min={w=50, h=20}, -- try to allow users to shrink your windows
Copy link
Member

Choose a reason for hiding this comment

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

are these numbers still appropriate?

resizable=true, -- if resizing makes sense for your dialog
resize_min={w=50, h=20}, -- try to allow users to shrink your windows
}

function PregnancyGui:init()
self.mother = false
if dfhack.gui.getSelectedUnit(true).sex == df.pronoun_type.she then
Copy link
Member

Choose a reason for hiding this comment

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

this will throw if there is no unit selected when the UI is opened. You should check for a nil return value from getSelectedUnit() before using it

self.mother = false
if dfhack.gui.getSelectedUnit(true).sex == df.pronoun_type.she then
self.mother = dfhack.gui.getSelectedUnit(true)
else self.mother = false
Copy link
Member

Choose a reason for hiding this comment

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

unless a field is always intended to be a boolean, it's better to leave unset values unset so they are nil instead of false.

@@ -26,14 +40,14 @@ function PregnancyGui:init()
},
widgets.HotkeyLabel{
frame={l=0},
label="Select Mother",
label="Set mother to selected unit",
Copy link
Member

Choose a reason for hiding this comment

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

you could also set enabled=function() local unit = dfhack.gui.getSelectedUnit(true) return unit and unit.sex == df.pronoun_type.she end,

Copy link
Member

Choose a reason for hiding this comment

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

and similarly for the father HotkeyLabel

options={{label='On', value=true, pen=COLOR_GREEN},
{label='Off', value=false, pen=COLOR_RED}},
initial_option=false
},
widgets.TooltipLabel{
Copy link
Member

Choose a reason for hiding this comment

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

Can you use a plain Label here? The attributes you're setting seem to undo everything that makes it a TooltipLabel

local term_index = {}
local months
for months=0,10 do
-- table.insert(term_options,{label=('%s months'):format(months),value=months}) --I tried this to add labels, probably doing something wrong, it broke the range widget
Copy link
Member

Choose a reason for hiding this comment

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

I notice below you were getting the label to index an array. this should work fine if you get the option value instead

@myk002
Copy link
Member

myk002 commented Jan 6, 2024

Is this PR still in progress?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants