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

loadEngineInjection #9294

Merged
merged 33 commits into from Nov 29, 2023
Merged

loadEngineInjection #9294

merged 33 commits into from Nov 29, 2023

Conversation

anselm
Copy link
Collaborator

@anselm anselm commented Nov 14, 2023

Summary

load engine injection

References

closes #insert number here

QA Steps

Copy link
Member

@speigg speigg left a comment

Choose a reason for hiding this comment

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

Typescript error:

src/behave-graph/nodes/Profiles/Engine/Events/onCollision.ts(105,5): error TS2304: Cannot find name 'startSystem'.

@@ -35,7 +35,7 @@ import { UUIDComponent } from '../components/UUIDComponent'

export const triggerEnter = (entity: Entity, triggerEntity: Entity, hit: ColliderHitEvent) => {
const triggerComponent = getComponent(triggerEntity, ColliderComponent)
if (!Array.isArray(triggerComponent.triggers)) return
if (!triggerComponent || !Array.isArray(triggerComponent.triggers)) return
Copy link
Member

Choose a reason for hiding this comment

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

rather than checking for the trigger component, we should add it to the collisionQuery, we can then ensure it exists

Copy link
Collaborator Author

@anselm anselm Nov 24, 2023

Choose a reason for hiding this comment

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

There is no concept TriggerComponent to query against. It is just a mode... and sometimes it is null. In fact avatars don't even have a ColliderComponent (or at least I get a runtime error occasionally that the avatar doesn't have this). I think the avatar is special and the root entity may not have that component - but instead there may be a collision capsule somewhere else in the avatar children graph?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It feels like that code has some kind of defect conceptually. Imagine a pairwise collision between a trigger volume and an avatar. The avatar in this case will be the triggerEntity (the entity that triggered the collision). This code is then looking through the avatar for triggers ...

Maybe the intention was to fetch the list of triggers from the trigger volume - so:

const triggerComponent = getComponent(triggerEntity, ColliderComponent)

Should be:

const triggerComponent = getComponent(entity, ColliderComponent)

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The risk of correcting or changing this is I don't have any kind of way of knowing if will impact anything. I don't know if our tests cover this case, or if this code is ever exercised. I don't think my code relies on this - and I don't know if there are any apps or demos that rely on it; or how to test it.

Copy link
Member

Choose a reason for hiding this comment

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

oh i see, i think this can be cleaned up and work as i said, now that we have CollisionComponent being generated when there is data and removed when not

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Um, ok, but there are conceptual issues still:

  • things entering that code don't always have a trigger -> either because avatar doesn't have one directly, OR because it is the receiving side of a triggerable interaction? which shouldn't need a trigger

  • that code may just be broken / may never have worked / may never have been tested? the test I added to block the crash may be actually due to testing the wrong object for a trigger?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it's best if I revert TriggerSystem.ts and treat that as a separate issue rather than trying to merge that in with this commit since this commit is languishing. The issue doesn't come up for me now due to fixes elsewhere, and examining TriggerSystem can be done as a separate commit.

@anselm anselm changed the title onCollision loadEngineInjection Nov 29, 2023
@HexaField HexaField merged commit 7ec4803 into dev Nov 29, 2023
8 checks passed
@HexaField HexaField deleted the oncomponentstate branch November 29, 2023 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants