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 wrong interface type returned for service regiatration #21

Merged
merged 1 commit into from
Aug 19, 2022

Conversation

FejZa
Copy link
Contributor

@FejZa FejZa commented Aug 19, 2022

Reality Collective - Reality Toolkit Pull Request

Overview

TypeExtensions.FindServiceInterfaceType returns the wrong interface type for a data provider when there is a more concrete type. This is actually the issue I was reporting the other day. So following scenario, here is the interface hierarchy for the toolkit's teleport providers:

IDashTeleportProvider -> ITeleportLocomotionProvider -> ILocomotionProvider
IBlinkTeleportProvider -> ITeleportLocomotionProvider -> ILocomotionProvider
IInstantTeleportProvider -> ITeleportLocomotionProvider -> ILocomotionProvider

Upon service registration TypeExtensions.FindServiceInterfaceType currently returns ITeleportLocomotionProvider for all three of them, which obbviously leads to issues as the service container will only register the first one and then fail to register the others since a type of that interface is already registered.
This is because Tyle.GetInterfaces() returns interfaces in an arbitrary order
And then whatever is the first one in the returned array that works for the service is seledcted
Which might not be the best suited candidate

Manual testing status

Manually tested.

@SimonDarksideJ
Copy link
Contributor

Interestingly this is one issue I've been working through on and off, so I'm curious to see if this resolves it.

However, you also need to test this on different platforms, not just editor. As I've found Unity REORDERS the type list depending on platform.

Can you supply test results on different platforms, e.g. Windows / UWP / Android?

Copy link
Contributor

@SimonDarksideJ SimonDarksideJ left a comment

Choose a reason for hiding this comment

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

Tested here with some of our trickier edge cases and this works like a charm

@FejZa FejZa merged commit aadeb07 into development Aug 19, 2022
@FejZa FejZa deleted the bugfix/interface-type branch August 19, 2022 22:29
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.

2 participants