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

feat: Item components #710

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Conversation

oierbravo
Copy link
Contributor

Feature

Basic implementation that allows extending ESCItem without touching the core.
We pass custom_data to the components by reference so components can have persistend data.

For now there is a super basic autoload for components... but i think, in the future, it should be done with an item_component_manager to check for name collisions and automatic initialization for the components without extending ESCItem.

With this implementation we are composing all our ESCItem customizations.

How to test

  • Open the demo game. Everything should work as always without errors in the log.

@BHSDuncan
Copy link
Collaborator

Thanks for this. I do have some thoughts/questions, though, that maybe you can help clarify:

I'm not exactly clear on the benefit of adding this in.

Given that the components don't depend at all on the parent (other than an optional Dictionary), you could either:

  • Simply add in components to your specific ESCItem since there's no real dependency; or,
  • Extend ESCItem when you create your own item scene and add in any registration functionality you may need (similar to what's in this PR).

If the issue is persistence, you can also use the second option above and pass on any already-persisted custom_data, which I believe you submitted a PR for a little while ago.

@StraToN what do you think? Am I missing something? Is this a feature that you think a lot of people would need?

@StraToN
Copy link
Collaborator

StraToN commented Oct 12, 2023

@StraToN what do you think? Am I missing something? Is this a feature that you think a lot of people would need?

I agree, I am also a bit dubitative on this one. @oierbravo could you please give us some more precision on the feature, and propose a few specific use cases. Is this different from the other PR your submitted a few weeks ago?

Also, does this change anything on the matter of save games?

@oierbravo
Copy link
Contributor Author

Hi!
We are trying to not extends the core ESCItem (and ESCPlayer) and trying to keep the core intact(kind of a blackbox).
This feature extends the other PR... it should be compatible with savegame as far as the ESCItem is.
I'm writing a little doc to explain everything with some examples😄.

Thanks for the super quick response and the care and attention to detail you put in this proyect😍

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.

3 participants