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

Imports UI changes from comp/2024. #268

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

barulicm
Copy link
Contributor

@barulicm barulicm commented Oct 2, 2024

No description provided.

Copy link

@gtrosawa gtrosawa left a comment

Choose a reason for hiding this comment

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

Mostly minor details that do not strictly require any action

Some questions on intent (e.g. overwriting an error code accumulator string before using its value somewhere) left as code comments

If they were indeed intended changes, I can go ahead and approve

@@ -128,6 +135,43 @@ export class AppState {
});
}

setIgnoreFieldSide(ignore_side: number) {
const state = this; // fix dumb javascript things
Copy link

Choose a reason for hiding this comment

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

this line seems to be unused
some other functions also have this same unused line
the functions that do use it do so for valid reasons (i.e. used to pass a reference to a function declaration, since the interpretation of "this" will be different when the function is called)

}

let pos = PIXI.Point;
pos = event.global;
Copy link

Choose a reason for hiding this comment

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

Suggest merge with line 139 to reduce confusion

},
rightClickDrag: function(event) {
const viewport = this.pixi.stage.getChildAt(0);
const ball_pos = viewport.getChildByName("ball").getChildAt(0).position;
Copy link

Choose a reason for hiding this comment

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

Seems to be unused
Is this deliberate?


this.state.sendSimulatorControlPacket(simulator_command);
},
rightClickDrag: function(event) {
Copy link

Choose a reason for hiding this comment

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

This function seems like it moves the ball to the cursor, and if dragged, draws the ball velocity vector, but it does not seem to update the state

endX: [-.12, -.1, .1, .12, .05],
endY: [-.03, .07, .07, -.03, -.09],
textX: [-.12, -.12, .12, .12, 0],
textY: [-.12, .14, .14, -.12, -.13]
}

// TODO: Figure out what order the motor numbers use
Copy link

Choose a reason for hiding this comment

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

0: FL
1: BL
2: BR
3: FR
4: DRB

@@ -171,6 +171,7 @@ export default {
if (general) errString = errString + "G";
if (hall) errString = errString + "H";
if (encoder) errString = errString + "E";
errString = String(i);
Copy link

Choose a reason for hiding this comment

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

Is this line intentional?
It seems like the error info will be stripped and replaced with the current index

@@ -7,6 +7,7 @@ import { Ball } from "@/ball"
import { Field, FieldDimensions, FieldSidedInfo } from "@/field"
import { AIState } from "@/AI"
import { Play } from "@/play"
import { exportDefaultSpecifier } from "@babel/types"
Copy link

Choose a reason for hiding this comment

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

Seems like this is an unused import

this.services["setIgnoreFieldSide"].callService(request,
function(result) {
if(!result.success) {
console.log("Failed to set ignore side: ", result.reason);
Copy link

Choose a reason for hiding this comment

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

Should probably be console.error or console.warn
Same with the other instances of logging unsuccessful requests

@barulicm barulicm mentioned this pull request Oct 15, 2024
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