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

Intake and Ejector Configurator #1017

Merged
merged 49 commits into from
Jul 16, 2024
Merged

Conversation

Dhruv-0-Arora
Copy link
Collaborator

@Dhruv-0-Arora Dhruv-0-Arora commented Jul 9, 2024

Description

Implementation of configuring the positioning of a robot's intake and ejector. Uses raycasting to detect the node (very basic implementation of detecting node) and saves the information. Allows user to scale the size of the sphere that spawns as well.

Features

  • Configure pickup zone
  • Configure ejector
  • Create physics sensor for pickup zone
  • Attach ejectable game piece
  • Eject ejectable game piece

Robot with configured pickup zone

image

Robot holding a Game Piece

image

Intake Configuration

image

Ejector Configuration

image

Temporary Components

These components are going to be left in and will be addressed shortly after in the scoring zone branch and likely one more branch to address input and behavior issues.

  • Pieces are picked up via a ray cast from the intake zone, 3 meters forward along the z axis.
  • Ejection is bound to left shift and control and isn't compatible with the InputSystem
  • Pickup zones are always rendered
  • There is no highlighting of rigid nodes

Warning

Merge after #1007

JIRA Issue

@Dhruv-0-Arora Dhruv-0-Arora requested review from HunterBarclay and a team as code owners July 9, 2024 00:32
@Dhruv-0-Arora Dhruv-0-Arora requested review from azaleacolburn and removed request for a team July 9, 2024 00:32
@Dhruv-0-Arora Dhruv-0-Arora marked this pull request as draft July 9, 2024 00:32
@Dhruv-0-Arora Dhruv-0-Arora self-assigned this Jul 9, 2024
@azaleacolburn azaleacolburn added the physics Relating to either the underlying physics engine or the usage of it label Jul 9, 2024
Comment on lines 211 to 215
private EnablePhysics() {
this._mirabufInstance.parser.rigidNodes.forEach(rn => {
World.PhysicsSystem.EnablePhysicsForBody(this._mechanism.GetBodyByNodeId(rn.id)!)
})
}
Copy link
Member

Choose a reason for hiding this comment

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

Why is this a function, but disabling physics for all rigid bodies in the same way isn't? It's not an issue, I'm just curious.

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 added that to solve a bug so that is a good point and ill add that. The problem is that all of that stuff is from PR #1009 so I might need to wait for that to be merged

Comment on lines 105 to 113
/**
* Enabing physics for a single body
*
* @param bodyId
*/
public EnablePhysicsForBody(bodyId: Jolt.BodyID) {
this._joltBodyInterface.ActivateBody(bodyId)
this.GetBody(bodyId).SetIsSensor(false)
}
Copy link
Member

Choose a reason for hiding this comment

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

5 lines of comments, 4 lines of code. This is what programming's all about. To be honest, these functions are completely self-documenting without any comments.

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 don't even know why I did that but I do know its gone from #1009 😇

fission/src/systems/physics/PhysicsSystem.ts Outdated Show resolved Hide resolved
if (isDragging) {
return
}
if (!event.value && !Array.from(this.transformControls.keys()).some(tc => tc.dragging)) {
Copy link
Member

Choose a reason for hiding this comment

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

Extracting this operation to a variable might be nice

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is true it would be more optimized

@LucaHaverty LucaHaverty mentioned this pull request Jul 9, 2024
3 tasks
@HunterBarclay
Copy link
Member

@Ewie21 Preferences will be merged in first, then this branch, then finally Ayla's branch will have all the glue for everything.

@azaleacolburn
Copy link
Member

Notes for Reviewers

  1. checkselectednode, listrobots, etc.. are duplicated functions. Where should I put them
  2. Can I optimized this in checkSelectedNode()
selectedRobot?.mirabufInstance?.parser.rigidNodes.forEach(rn => {
            if (World.PhysicsSystem.GetBody(selectedRobot.mechanism.GetBodyByNodeId(rn.id)!).GetID() === body.GetID()) {
                bodyAttachmentRef.current = body
                returnValue = true
            }
        })

You could make it into a regular for loop and break, since all you'll do is set the same variables again. Also, could you just compare the ids directly, or are ids not consistent between Physics System and mechanism? I'm sure there are other things as well, but I haven't throughly read through the code yet.

@HunterBarclay
Copy link
Member

HunterBarclay commented Jul 15, 2024

You could make it into a regular for loop and break, since all you'll do is set the same variables again. Also, could you just compare the ids directly, or are ids not consistent between Physics System and mechanism? I'm sure there are other things as well, but I haven't throughly read through the code yet.

@Ewie21 This no longer exists.

@HunterBarclay
Copy link
Member

Note: Rigid node highlighting can be implemented once #1023 is merged in.

@LucaHaverty
Copy link
Collaborator

LucaHaverty commented Jul 15, 2024

I'm not sure when this was introduced, but opening the poker panel or settings modal throws this error. It might be anything that uses a slider.

TypeError: Cannot read properties of undefined (reading 'toLocaleString')
    at Slider (Slider.tsx:38:44)
    at renderWithHooks (react-dom.development.js:15486:18)
    at mountIndeterminateComponent (react-dom.development.js:20103:13)
    at beginWork (react-dom.development.js:21626:16)
    at HTMLUnknownElement.callCallback2 (react-dom.development.js:4164:14)
    at Object.invokeGuardedCallbackDev (react-dom.development.js:4213:16)
    at invokeGuardedCallback (react-dom.development.js:4277:31)
    at beginWork$1 (react-dom.development.js:27490:7)
    at performUnitOfWork (react-dom.development.js:26596:12)
    at workLoopSync (react-dom.development.js:26505:5)

@HunterBarclay
Copy link
Member

HunterBarclay commented Jul 15, 2024

@LucaHaverty Oops, I'll take care of it

Copy link
Collaborator

@LucaHaverty LucaHaverty left a comment

Choose a reason for hiding this comment

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

Besides the slider things, I could not find any other issues, great work!

@@ -12,16 +12,15 @@ type SliderProps = {
label?: string
min: number
max: number
defaultValue: number
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps this is the cause of the slider issues? Maybe due to a merge?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@LucaHaverty could you look at that again and see if you can find errors with the fix
I didn't touch the ZoneConfigPanel because I don't want merge conflicts from your PR

Added changes to other files but ignored ZoneConfigPanel due to PR #1021 changing it
Copy link
Collaborator

@LucaHaverty LucaHaverty left a comment

Choose a reason for hiding this comment

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

Everything worked well, fantastic work!

if (isDragging) {
return
}
if (!event.value && !Array.from(this.transformControls.keys()).some(tc => tc.dragging)) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is true it would be more optimized

@@ -12,16 +12,15 @@ type SliderProps = {
label?: string
min: number
max: number
defaultValue: number
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@LucaHaverty could you look at that again and see if you can find errors with the fix
I didn't touch the ZoneConfigPanel because I don't want merge conflicts from your PR

fission/src/mirabuf/IntakeSensorSceneObject.ts Outdated Show resolved Hide resolved
fission/src/mirabuf/MirabufSceneObject.ts Outdated Show resolved Hide resolved
Copy link
Member

@BrandonPacewic BrandonPacewic left a comment

Choose a reason for hiding this comment

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

@Dhruv-0-Arora Are you not the author of this PR? Or did @HunterBarclay take this one over?

@Dhruv-0-Arora
Copy link
Collaborator Author

@Dhruv-0-Arora Are you not the author of this PR? Or did @HunterBarclay take this one over?

Hunter is the owner for this one

Copy link
Collaborator

@a-crowell a-crowell left a comment

Choose a reason for hiding this comment

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

Works great! Just removed an unused import.

Copy link
Member

@BrandonPacewic BrandonPacewic left a comment

Choose a reason for hiding this comment

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

Want to make sure @Dhruv-0-Arora 's comments are looked at before merge.

could you do gpAssoc?.isGamePiece

Originally posted by @Dhruv-0-Arora in #1017 (comment)

@LucaHaverty could you look at that again and see if you can find errors with the fix
I didn't touch the ZoneConfigPanel because I don't want merge conflicts from your PR

Originally posted by @Dhruv-0-Arora in #1017 (comment)

That is true it would be more optimized

Originally posted by @Dhruv-0-Arora in #1017 (comment)

@HunterBarclay HunterBarclay merged commit 96185ad into dev Jul 16, 2024
12 checks passed
@HunterBarclay HunterBarclay deleted the dhruv/1728/intake-ejector-config branch July 16, 2024 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
physics Relating to either the underlying physics engine or the usage of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants