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

Decouple Wieldables from player scene #150

Closed
brian-holsters opened this issue Apr 5, 2024 · 3 comments · Fixed by #151
Closed

Decouple Wieldables from player scene #150

brian-holsters opened this issue Apr 5, 2024 · 3 comments · Fixed by #151
Labels
enhancement New feature or request

Comments

@brian-holsters
Copy link
Contributor

As a general enhancement, I'd suggest making it not required for the wieldable scenes to be pre-instantiated in the player scene:
image

Making a few changes this should be pretty simple to do and would ease the process of creating new wieldables.

I'll submit a proof of concept PR showing how I would personally go about this.

@Phazorknight Phazorknight added the enhancement New feature or request label Apr 5, 2024
@brian-holsters
Copy link
Contributor Author

I'm having some issues dealing with the ammo wieldables, since the relationship between a weapon and its ammo seems to be separated depending on the interaction.

One of the main issues i've also had during the refactoring is that godot really doesn't like leaving a variable but removing its export property, but I've managed to get around that.

I'll keep working on this tomorrow, but my general workflow has been:

  • Remove all the wieldables from the player scene, the first step to change is breaking stuff
  • Add a reference (through an export) to the Wieldables node in the PlayerInteractionComponent, we will use this later to add and remove the wieldable instances as necessary.
  • Add PackedScene to the item resource (at first this causes an infinite recursion error)
  • To solve the recursion issue, remove the resource reference that already exists in the Wieldable scene, since it won't be necessary.
  • Modify the equip_wieldable function in PlayerInteractionComponent, it actually becomes easier to understand with this change, since I removed the need for the loop entirely by instantiating the wieldable PackedScene stored in the item resource.

With all of this done, i have one recursion error left which has to do with reloading a weapon through combining it with the ammo in the inventory, but i think i know how i'll solve this.

All in all, you've got a pretty impressive project going here and while there is a lot of coupling going on between different pieces, it seems like it should be mostly solvable, and this seems like a real fun project to dump some free time in if you're open to contributions :)

@Phazorknight
Copy link
Owner

All in all, you've got a pretty impressive project going here and while there is a lot of coupling going on between different pieces, it seems like it should be mostly solvable, and this seems like a real fun project to dump some free time in if you're open to contributions :)

Thank you! And absolutely open to contributions!
I've tried to make Cogito flexible but also intuitive to use/modify once the user understands its structure.
I'm sure there are quite a few spots where nodes could be decoupled more / instanced dynamically.

With all of this done, i have one recursion error left which has to do with reloading a weapon through combining it with the ammo in the inventory, but i think i know how i'll solve this.

Oh that's odd, since AFAIK this wouldn't touch any parts of CogitoWieldable / Wieldable nodes at all. But I know items referencing each others can definitely create recursions.

@brian-holsters
Copy link
Contributor Author

I hoped for this change to require less work honestly, but i think it came out pretty neat and it definitely helped me figure out how some parts of the project interact with eachother. I did notice some inconsistencies in the way you're dealing with scene references in exports though, since in some spots you are exporting PackedScenes and in other spots they are paths to the scene file, which are then loaded at runtime.

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
None yet
Development

Successfully merging a pull request may close this issue.

2 participants