8000 Ir 8616 revive bounding box by danskate · Pull Request #2018 · ir-engine/ir-engine · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Ir 8616 revive bounding box #2018

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 6 commits into
base: dev
Choose a base branch
from
Open

Ir 8616 revive bounding box #2018

wants to merge 6 commits into from

Conversation

danskate
Copy link
Contributor
@danskate danskate commented Jun 3, 2025 •
edited
Loading

Summary

This change adds a BoundingBoxComponent to any GLTFComponent and fixes the BoundingBox calculation.

Subtasks Checklist

Breaking Changes

References

closes #insert number here

QA Steps

Before Fix:
https://github.com/user-attachments/assets/eb0cd4da-ce7f-40cc-90f2-1019bef407d3

After Fix:
https://github.com/user-attachments/assets/ab697f55-c2de-409d-8455-f17ede082ae8

@danskate danskate force-pushed the ir-8616-bounding-box branch from 0eb50fd to 02ccab7 Compare June 4, 2025 00:06
HexaField
HexaField requested changes Jun 4, 2025
View reviewed changes
@danskate
Copy link
Contributor Author
danskate commented Jun 4, 2025

I pushed new changes that introduced a new worldBox variable to the BoundingBoxComponent. Some uses require local or world space bounding boxes so I made .box local and added worldBox to be the world space box.

@danskate danskate force-pushed the ir-8616-bounding-box branch from d8bfd57 to 0b7ec76 Compare June 4, 2025 22:20
@danskate danskate marked this pull request as ready for review June 4, 2025 22:55
HexaField
HexaField requested changes Jun 5, 2025
View reviewed changes
Copy link
Member
@HexaField HexaField left a comment

Choose a reason for hiding this comment

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

we should probably split these into AABBComponent and OABBComponent for axis aligned and object aligned respectively, though i'm willing to keep it as is. it can become a performance concern in larger scenes, especially if updated dynamically.

@HexaField
Copy link
Member

what use case do we have currently for OABBs? my expectation previously was AABB

@HexaField HexaField mentioned this pull request Jun 6, 2025
Open
noodleA1 added a commit that referenced this pull request Jun 7, 2025
@noodleA1
Integrate Dan's bounding box infrastructure and implement fallback ap…
24ea93c 
…proach

- Merge Dan's PR #2018 (ir-8616-bounding-box) with worldBox/box distinction
- Remove auto-BoundingBoxComponent creation from MeshComponent (use GLTF approach)
- Implement getEntityBoundingBox fallback: BoundingBoxComponent.worldBox OR MeshComponent.geometry.boundingBox
- Update alignObjects to use worldBox for world-space calculations
- Follow HexaField's guidance: no auto-creation, proper fallback, AABB handling

Addresses HexaField's review comments and integrates with Dan's proper bounding box infrastructure.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HexaField HexaField HexaField requested changes

@SamMazerIR SamMazerIR Awaiting requested review from SamMazerIR SamMazerIR is a code owner

@speigg speigg Awaiting requested review from speigg speigg is a code owner

@AidanCaruso AidanCaruso Awaiting requested review from AidanCaruso AidanCaruso is a code owner

@MichaelEstes MichaelEstes Awaiting requested review from MichaelEstes MichaelEstes is a code owner

@dinomut1 dinomut1 Awaiting requested review from dinomut1 dinomut1 is a code owner

@aditya-mitra aditya-mitra Awaiting requested review from aditya-mitra aditya-mitra is a code owner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@danskate @HexaField
10C9
0