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

fix: Correctly get inventory objects registered in other rooms #608

Merged
merged 1 commit into from
Jul 14, 2022

Conversation

MatteoPiovanelli
Copy link
Contributor

This fixes godot-escoria/escoria-issues#280

As I mentioned on Discord, when an object is added to the inventory it is added to an array for the room the player is currently in. When get_object got called, it would only look in the current room, skipping all possible inventory items from other rooms.

This fix gets around that, specifically for items that are registered in the inventory.

I'm opening this as a draft because I'm not 100% this is the entirely correct solution. (and on top of that, I'm not 100% I followed the contribution guidelines correctly)
A better approach could be to have a separate array for inventory items. I noticed it's not there, and I don't know the history of the reasoning why.
Alternatively, yet another approach could be to have a similar loop through the objects in the rooms, but to wrap that in a func in the inventory manager, to "hide" its implementation details from the object manager.

@MatteoPiovanelli MatteoPiovanelli changed the title Correctly get inventory objects registered in other rooms fix(get_object): Correctly get inventory objects registered in other rooms Jul 12, 2022
@MatteoPiovanelli MatteoPiovanelli marked this pull request as ready for review July 12, 2022 20:48
@StraToN
Copy link
Collaborator

StraToN commented Jul 13, 2022

I'll test the branch before reviewing. Sorry for the delay

@StraToN
Copy link
Collaborator

StraToN commented Jul 14, 2022

Ok, the fix does fix the issue great, thanks a lot!

I am not exactly certain why we didn't do that already: maybe I wanted to have all inventory items to be automatically registered at each room change, so that they are accessible through object_manager.room_objects array anytime? Anyway, the right way was to use object_manager.get_object() method so this fix is ok to me.

Approved. Thanks a lot!

@StraToN StraToN changed the title fix(get_object): Correctly get inventory objects registered in other rooms fix: Correctly get inventory objects registered in other rooms Jul 14, 2022
@StraToN StraToN merged commit 324ef5f into godot-escoria:develop Jul 14, 2022
@StraToN
Copy link
Collaborator

StraToN commented Jul 14, 2022

Thanks !

@MatteoPiovanelli MatteoPiovanelli deleted the issue-280 branch July 14, 2022 19:59
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.

Combined Item Causes Issues after Moving between Rooms
3 participants