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

TryFindComponentOnEntityContainerOrParent no fail please #5322

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

IProduceWidgets
Copy link

@IProduceWidgets IProduceWidgets commented Jul 29, 2024

TryFindComponentOnEntityContainerOrParent
would fail trying to Resolve on parents that don't have the component its searching for. This prevents that by changing the Resolve to TryComp.

@IProduceWidgets IProduceWidgets changed the title TryFindComponentsOnEntityContainerOrParent no fail please TryFindComponentOnEntityContainerOrParent no fail please Aug 7, 2024
@ElectroJr
Copy link
Member

Isn't the trycomp just equivalent to the old resolve? Unless people are passing in components via the ref? I have no idea why this method even takes in a ref comp

@deltanedas
Copy link
Contributor

yeah shouldnt foundComponent be out instead of ref

@IProduceWidgets
Copy link
Author

IProduceWidgets commented Aug 7, 2024

Agreed, I switched it to out and tested that it works recursively as expected.

Fortunately I think I'm the only person who has tried to use this on content so there's nothing to fix!

@IProduceWidgets
Copy link
Author

IProduceWidgets commented Aug 12, 2024

Isn't the trycomp just equivalent to the old resolve? Unless people are passing in components via the ref? I have no idea why this method even takes in a ref comp

I have no idea why it was using ref. I don't think there's any uses of the method on content, probably because of its scrungliness, and the method that returns all found parent components instead of the first one doesn't use ref and also uses trycomp instead. As for if they're functionally different, the resolve throws an assert if any of the parents while recursing don't have the component, which is no good here. Also as ref it required you to pass a ref, which is not needed imo, and unintuitive.

@IProduceWidgets
Copy link
Author

Bump.
It's just a bugfix, not new functionality.

Copy link
Member

@PJB3005 PJB3005 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a minor breaking change which I'd rather not merge. I do get that out makes more sense but what's done is done, it should be possible to implement the function regardless.

@IProduceWidgets
Copy link
Author

Hokay, made it ref again to avoid the breaky.

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.

4 participants