Skip to content

Dnd5e biography support #113

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

Open
wants to merge 3 commits into
base: dnd5e-biography-support
Choose a base branch
from

Conversation

cyelis1224
Copy link
Collaborator

@cyelis1224 cyelis1224 commented Nov 24, 2024

ezgif-1-f6e577b661

Added a checkbox to the Unkenniness Parameters so that DM can choose to include the biography section of the actors sheet in the preamble being sent to the LLM. This makes it where if the actor already has a Biography you can just add some additional instructions in the preamble to explain how the NPC should talk or behave while handling their backstory from their Bio.

This was made as a response to my feature request #111

@TheComamba
Copy link
Owner

Are you willing to add some tests like "bio is included if boolean is set", "included bio and empty preamble is ok", "included empty bio and empty preamble is error", "empty preamble and no bio inclusion is error", or shall I?


console.log("Constructed Preamble:", preamble);

request = `${preamble}\n\n${request}`;
Copy link
Owner

Choose a reason for hiding this comment

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

This way the preamble is included twice:
generateResponse calls collectChatMessages, which calls messagesOrganisedForTemplate, which prepends the preamble.

@cyelis1224
Copy link
Collaborator Author

I can handle it. I'll take care of it tomorrow.

@cyelis1224
Copy link
Collaborator Author

cyelis1224 commented Nov 27, 2024

So I can split this out if need be, but I made a mistake and made these changes in another branch I had. (Still not the most proficient with GitHub haha) I just wanted to show I got the biography working like you requested. I just accidentally did it in another branch that I had for reworking the way conversations are handled by the LLM. -_-

The sad part is, as far as I could figure out, in order to have the flags on the LLM's response we lose the ability to do the /commands like whisper and /talk. I'll split it if you don't want to include all this for now but wanted to show you the changes I had to make to get multi-user convo's working and some finer control over the LLM's instructions.

@cyelis1224
Copy link
Collaborator Author

I went ahead and split it, I also removed the checkbox as it felt redundant since we are checking for one the other or both now.

had left the check box option by accident
@@ -31,6 +31,12 @@ async function getGenerationParameters(actor) {
}
let params = {};
params.actorName = actor.name;

params.includeBiography = await actor.getFlag("unkenny", "includeBiography") || false;
Copy link
Owner

Choose a reason for hiding this comment

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

Without the checkbox, this flag has become obsolete.

@TheComamba
Copy link
Owner

Really, this breaks /whisper and /talk? As far as I remember, /whisper is evaluatee by foundry, and thus at a completely dirferent location than /talk. I also seem to remember that the evaluation of the /talk macro was a bit hacky, but that information may be outdated or misremembered. Maybe I can have a look at the weekend. If you have any preliminary tests that you can push feel free to do so. Then we can avoid doing the work twice.

@cyelis1224
Copy link
Collaborator Author

cyelis1224 commented Nov 28, 2024

Alright, I'll see what I can figure out.

I saw in the issue thread for the chat window conversation we were having you said you were assuming I was a better front end dev. I highly doubt that, I work in construction and just play with JS and code as a hobby.

I was one of the earlier contributors for the AboveVTT project (A VTT extension that overlays a VTT directly in dndbeyond.com) but I swapped to foundry a while back and recently started playing with making modules for Foundry. Happened across your project and thought it was interesting and was just playing around haha.

@TheComamba TheComamba changed the base branch from main to dnd5e-biography-support December 1, 2024 17:22
@TheComamba
Copy link
Owner

Since you are a contributor now, I suggest that we merge these changes to a branch in the main repository. This will make, well, collaboration easier, as we can both have the same repository cloned locally. If it's allright with you.
I think you should have the permissions to complete this PR yourself, because I changed the target branch to something other than main. However, GitHub will only allow squash merges. If you insist on keeping your history before we go to main I can temporarily disable this rule.

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