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

Adv-Rumors additional conversation options #1182

Merged
merged 12 commits into from
Jul 2, 2024

Conversation

Crystalwarrior
Copy link
Contributor

Adds "ask whereabouts of" conversation options for ALL your histfig relationships

@myk002
Copy link
Member

myk002 commented Jun 21, 2024

is this PR still a draft or is it ready for review?

@Crystalwarrior Crystalwarrior marked this pull request as ready for review June 22, 2024 13:59
@Crystalwarrior
Copy link
Contributor Author

@myk002 ready for review

internal/advtools/convo.lua Outdated Show resolved Hide resolved
local function addHistFigWhereaboutsChoice(profile)
for i, c in pairs(adventure.conversation.conv_choice_info) do
if c.cc.type == df.talk_choice_type.AskWhereabouts and
c.cc.invocation_target_hfid ~= -1 and
Copy link
Member

Choose a reason for hiding this comment

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

if profile.histfig_id is never equal to -1, then you can remove the check for -1

local caste = creature.caste[histfig.caste]
name = caste.caste_name[0]
end
local title = "Ask for the whereabouts of the " .. name .. " " .. dfhack.TranslateName(histfig.name, true)
Copy link
Member

Choose a reason for hiding this comment

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

do we only need the english name here? are they never referenced by their native name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In vanilla convo system yeah they're only referenced by their English name

Copy link
Member

Choose a reason for hiding this comment

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

is that what you want, though? @Ozzatron what do you think?

if profile._type == df.relationship_profile_hf_historicalst then
title = title .. " (Heard of)"
end
local choice = new_choice(df.talk_choice_type.AskWhereabouts, title, dfhack.TranslateName(histfig.name):split())
Copy link
Member

Choose a reason for hiding this comment

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

why are we adding keywords for the untranslated name if the translated name appears in the text itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Vanilla keyword behavior

Copy link
Member

Choose a reason for hiding this comment

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

and the translated name is already in the keyword list?

internal/advtools/convo.lua Outdated Show resolved Hide resolved
local function addIdentityWhereaboutsChoice(identity)
for i, c in pairs(adventure.conversation.conv_choice_info) do
if c.cc.type == df.talk_choice_type.AskWhereabouts and
c.cc.invocation_target_hfid ~= -1 and
Copy link
Member

Choose a reason for hiding this comment

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

same as above -- if identity.impersonated_hf is never -1, then we don't need this check

Copy link
Contributor Author

@Crystalwarrior Crystalwarrior Jun 30, 2024

Choose a reason for hiding this comment

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

It's possible for impersonated hf identity to be -1

Comment on lines 115 to 135
local identity_name = identity.name
local name = ""
local creature = df.creature_raw.find(identity.race)
if creature then
local caste = creature.caste[identity.caste]
name = caste.caste_name[0]
else
-- no race given for the identity, assume it's the histfig
local histfig = df.historical_figure.find(identity.histfig_id)
creature = df.creature_raw.find(histfig.race)
if creature then
local caste = creature.caste[histfig.caste]
name = caste.caste_name[0]
end
end
local title = "Ask for the whereabouts of the " .. name .. " " .. dfhack.TranslateName(identity_name, true)
local choice = new_choice(df.talk_choice_type.AskWhereabouts, title)
-- insert before the last choice, which is usually "back"
adventure.conversation.conv_choice_info:insert(#adventure.conversation.conv_choice_info-1, choice, dfhack.TranslateName(identity_name):split())
choice.cc.invocation_target_hfid = identity.impersonated_hf
end
Copy link
Member

Choose a reason for hiding this comment

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

this duplicates too much of the logic in addHistFigWhereaboutsChoice; please refactor

Comment on lines 152 to 165
for _, profile in pairs(visual) do
addHistFigWhereaboutsChoice(profile)
end

-- This option will likely always fail unless the false identity is impersonating someone
-- but giving away the false identity's true historical figure feels cheap.
for _, profile in pairs(identity) do
addIdentityWhereaboutsChoice(df.identity.find(profile.identity_id))
end

-- Historical entities go last so as to not give away fake identities
for _, profile in pairs(historical) do
addHistFigWhereaboutsChoice(profile)
end
Copy link
Member

Choose a reason for hiding this comment

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

with the loop through adventure.conversation.conv_choice_info, each of these has quadradic runtime complexity. perhaps it would be a good idea to make a single loop through adventure.conversation.conv_choice_info and cache the results? (and update the cache as you add more entries)

Copy link
Member

Choose a reason for hiding this comment

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

also, ipairs is semantically appropriate for vectors, not pairs

end
end

-- generate extra keywords for all options
Copy link
Member

Choose a reason for hiding this comment

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

if this is being run unconditionally down here, why are you adding keywords in new_choice? can the call to add_keywords be removed from new_choice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is because new_choice might have its own "vanilla"-style added keywords which is just a few, while this section ensures more searchability

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.

previous comments appear to be unaddressed

@Crystalwarrior Crystalwarrior requested a review from myk002 July 1, 2024 20:49
changelog.txt Outdated Show resolved Hide resolved
@myk002 myk002 merged commit 7301bdf into DFHack:master Jul 2, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants