Phazorknight / Cogito

Immersive Sim Template Project for GODOT 4
MIT License
873 stars 100 forks source link

Decouple Wieldables from player scene #150

Closed brian-holsters closed 6 months ago

brian-holsters commented 6 months ago

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.

brian-holsters commented 6 months ago

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:

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 commented 6 months ago

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 commented 6 months ago

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.