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

LLMClient Initialization Issue with OnEnable #20

Closed
BruceKristelijn opened this issue Jan 13, 2024 · 4 comments · Fixed by #28 or #27
Closed

LLMClient Initialization Issue with OnEnable #20

BruceKristelijn opened this issue Jan 13, 2024 · 4 comments · Fixed by #28 or #27
Labels
enhancement New feature or request
Milestone

Comments

@BruceKristelijn
Copy link
Contributor

Hi there. I have tried implementing the project and it works very good so far. Thanks and props to you!
However in our project we create LLMClient's dynamically using AddComponent. However the usage of OnEnable causes some issues with this. OnEnable is called immediately when gamobject.AddComponent is called. Because of this settings the prompt has some issues.

I had this issue with Unity's own AudioSources aswell and a workaround I use is turning the game object off, adding the component and turning the gameobject on again.

gameObject.SetActive(false);
llmclient = gameObject.AddComponent<LLMClient>();
llmclient.prompt = suspectInstance.Suspect.GeneratePersona();
gameObject.SetActive(true);

I would suggest using Awake or the Start methods instead of OnEnable as there is no behaviour like OnDisable.

@amakropoulos
Copy link
Collaborator

Thank you for the contribution 👏 !
I didn't think about that, good point.
I'd rather not add it in the Start because I'd like the server to start before any script calling it, but both OnEnable and Awake are both fine with me.
I'll test the PR and add it in v1.0.2!

@amakropoulos amakropoulos added the enhancement New feature or request label Jan 15, 2024
@amakropoulos amakropoulos moved this to In Progress in LLM for Unity Roadmap Jan 15, 2024
@amakropoulos amakropoulos added this to the v1.0.2 milestone Jan 15, 2024
@BruceKristelijn
Copy link
Contributor Author

Thanks, about the start function is a great point! I am unsure is moving to awake is the best. What I was also thinking was maybe to create a setting like enableOnAwake or startOnEnable etc. That will be set true by default by the custom editor but the default value is false.

@amakropoulos
Copy link
Collaborator

Good point. I don't think there is a way to pass an argument when adding a component dynamically though (with AddComponent<LLMClient>()).
I have added your implementation as well as your example in the Readme.md here.
Let me know if you have any comments!

@amakropoulos amakropoulos moved this from In Progress to Implemented in LLM for Unity Roadmap Jan 16, 2024
@github-project-automation github-project-automation bot moved this from Implemented to Done in LLM for Unity Roadmap Jan 18, 2024
@amakropoulos
Copy link
Collaborator

@BruceKristelijn this is now merged in v1.0.2!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants