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

Fixed physics collider visualization offset #555

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

Conversation

erikrl2
Copy link

@erikrl2 erikrl2 commented Jun 11, 2022

Describe the issue

The collider visualization rectangle does not match the actual position of the collider when using offset:

Hazel - Physics Collider Offset Issue - before

PR impact

List of related issues/PRs this will solve:

Impact Issue/PR
Issues this solves None
Other PRs this solves None

Proposed fix

Adding the offset and rotation to the Box2D box shape:

b2PolygonShape boxShape;
boxShape.SetAsBox(bc2d.Size.x * transform.Scale.x, bc2d.Size.y * transform.Scale.y,
	b2Vec2(bc2d.Offset.x, bc2d.Offset.y), bc2d.Rotation);

Calculating the relative transform of the visualization rectangle before the entity's transform:

glm::vec3 scale = tc.Scale * glm::vec3(bc2d.Size * 2.0f, 1.0f);

glm::mat4 transform = glm::translate(glm::mat4(1.0f), tc.Translation)
	* glm::rotate(glm::mat4(1.0f), tc.Rotation.z, glm::vec3(0.0f, 0.0f, 1.0f))
	* glm::translate(glm::mat4(1.0f), glm::vec3(bc2d.Offset, 0.001f))
	* glm::rotate(glm::mat4(1.0f), bc2d.Rotation, glm::vec3(0.0f, 0.0f, 1.0f))
	* glm::scale(glm::mat4(1.0f), scale);

Now it looks like:

Hazel - Physics Collider Offset Issue - after

Also fixed this for circle colliders.

Note:
Needed to add Rotation attribute to BoxCollider2DComponent. You could expose this to the editor and serialize it.

@erikrl2 erikrl2 changed the title Fixed physics collider offset location Fixed physics collider visualization rectangle location Jun 13, 2022
@erikrl2 erikrl2 changed the title Fixed physics collider visualization rectangle location Fixed physics collider visualization offset Jun 17, 2022
(the collider radius is only dependent on the entity's x-scale)
(the collider radius is only dependent on the entity's x-scale)
@erikrl2 erikrl2 changed the base branch from physics to master June 20, 2022 16:41
@erikrl2
Copy link
Author

erikrl2 commented Jun 24, 2022

Commit history is such a mess because I failed rebasing to master.

@TheCherno TheCherno mentioned this pull request Jun 28, 2022
8 tasks
@@ -120,6 +120,7 @@ namespace Hazel {
{
glm::vec2 Offset = { 0.0f, 0.0f };
glm::vec2 Size = { 0.5f, 0.5f };
float Rotation = 0.0f;
Copy link
Owner

Choose a reason for hiding this comment

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

Why has this Rotation variable been added to BoxCollider2DComponent? It doesn't seem to have any UI for setting it anywhere, so it seems like it would just always be 0.

Copy link
Owner

Choose a reason for hiding this comment

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

Remove this.

Copy link

@AkliDev AkliDev Oct 29, 2022

Choose a reason for hiding this comment

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

I think a box collider should be able to be rotated independently. Instead of removing rotation. You can add the field in SceneHierarchyPanel and make it serializable.

Hazel/src/Hazel/Scene/Scene.cpp Outdated Show resolved Hide resolved
Hazelnut/src/EditorLayer.cpp Outdated Show resolved Hide resolved
@@ -120,6 +120,7 @@ namespace Hazel {
{
glm::vec2 Offset = { 0.0f, 0.0f };
glm::vec2 Size = { 0.5f, 0.5f };
float Rotation = 0.0f;
Copy link
Owner

Choose a reason for hiding this comment

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

Remove this.

@erikrl2
Copy link
Author

erikrl2 commented Jul 4, 2022

I applied your requested changes @TheCherno 👍

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