Introduce arrows #82

Merged
maxime.batista merged 12 commits from editor/arrows into master 1 year ago

This pull request adds the ability to drag and drop an arrow from a player to another player or anywhere on the court.

Arrows can be bend using control points, that can be added by double-clicking on an arrow.

The arrow types are automatically detected and changed depending on the player states :

  • If a player has the ball in his hand :

    • if the user drag and drops an arrow to another player : it is a throw, a straight dotty arrow is used to represent the shot.
    • If the user drag and drops an arrow anywhere else on the court : it is a dribble, a wavy arrow is used
  • If a player does not have the ball in his hand :

    • If the user drag and drops an arrow to another player : it is a screen
    • If the user drag and drops an arrow anywhere else on the court : it is a simple movement

here's an example :
image

This pull request adds the ability to drag and drop an arrow from a player to another player or anywhere on the court. Arrows can be bend using control points, that can be added by double-clicking on an arrow. The arrow types are automatically detected and changed depending on the player states : * If a player has the ball in his hand : * if the user drag and drops an arrow to another player : it is a throw, a straight dotty arrow is used to represent the shot. * If the user drag and drops an arrow anywhere else on the court : it is a dribble, a wavy arrow is used * If a player does not have the ball in his hand : * If the user drag and drops an arrow to another player : it is a screen * If the user drag and drops an arrow anywhere else on the court : it is a simple movement here's an example : ![image](/attachments/101325c0-b5fc-4e0b-a91e-f5354d4c6e16)
maxime.batista added 10 commits 1 year ago
maxime.batista requested review from clement.freville2 1 year ago
maxime.batista requested review from yanis.dahmane-bounoua 1 year ago
maxime.batista requested review from vivien.dufour 1 year ago
maxime.batista requested review from mael.daim 1 year ago
maxime.batista requested review from samuel.berion 1 year ago
vivien.dufour approved these changes 1 year ago
clement.freville2 requested changes 1 year ago
clement.freville2 left a comment

l.objects is undefined (isBallOnCourt) on https://maxou.dev/IQBall/editor/arrows/public

`l.objects is undefined` (`isBallOnCourt`) on https://maxou.dev/IQBall/editor/arrows/public
parentBase,
)
const setControlPointPos = (newPos: Pos | undefined) => {

The idiomatic way to think about that is that null is the explicit absence of something, whereas undefined is the "I haven't a value for that yet so you shouldn't use me".

In your case (newPos: Pos | null) would be more appropriated.
undefined is effectively an erro type or a way to declare a parameter as optional (newPos?: Pos).

The idiomatic way to think about that is that `null` is the explicit absence of something, whereas `undefined` is the "I haven't a value for that yet so you shouldn't use me". In your case `(newPos: Pos | null)` would be more appropriated. `undefined` is effectively an erro type or a way to declare a parameter as optional `(newPos?: Pos)`.
maxime.batista marked this conversation as resolved
return Math.sqrt((a.x - b.x) ** 2 + (a.y - b.y) ** 2)
}
export function size(vector: Pos): number {

The length of a vector is called its magnitude or its norm.

The length of a vector is called its magnitude or its norm.
maxime.batista marked this conversation as resolved
*/
export function angle(a: Pos, b: Pos): number {
const r = relativeTo(a, b)
return Math.atan2(r.x, r.y)

The atan2 function arguments are inverted.

The `atan2` function arguments are inverted.
Poster

i know but i did this because i would negate the result everywhere otherwise

i know but i did this because i would negate the result everywhere otherwise
maxime.batista marked this conversation as resolved
}
}
export function between(a: Pos, b: Pos): Pos {

This "between" is a middle of a segment.

This "between" is a middle of a segment.
maxime.batista marked this conversation as resolved
}
}
export function rotate(vec: Pos, deg: number): Pos {

This function takes a angle in degrees, whereas the Math functions use radians.

This function takes a angle in degrees, whereas the `Math` functions use radians.
maxime.batista marked this conversation as resolved
export function rotate(vec: Pos, deg: number): Pos {
return {
x: Math.cos(deg * vec.x) - Math.sin(deg * vec.y),
y: Math.sin(deg * vec.x) + Math.cos(deg * vec.y),

This formula is incorrect.
x_2 = cos(\alpha)x_1 - sin(\alpha)y_1
y_2 = sin(\alpha)x_1 + cos(\alpha)y_1

This formula is incorrect. $x_2 = cos(\alpha)x_1 - sin(\alpha)y_1$ $y_2 = sin(\alpha)x_1 + cos(\alpha)y_1$
maxime.batista marked this conversation as resolved
import React from "react"

Unused import

Unused import
maxime.batista marked this conversation as resolved
import { NULL_POS, ratioWithinBase } from "../arrows/Pos"
export interface PlayerProps {
export interface PlayerProps<A extends ReactNode> {

Don't. A component should never rely on the type behind ReactNode.

Don't. A component should **never** rely on the type behind `ReactNode`.
maxime.batista marked this conversation as resolved
return (args: A) => {
clearTimeout(task)
return new Promise((resolve) => {
task = setTimeout(() => f(args).then(resolve), delay)

The inner promise should also be catched.

The inner promise should also be catched.
maxime.batista marked this conversation as resolved
package.json Outdated
"react": "^18.2.0",
"react-dom": "^18.2.0",
"react-draggable": "^4.4.6",
"react-xarrows": "^2.0.2",

Unused dependency

Unused dependency
maxime.batista marked this conversation as resolved
package.json Outdated
"devDependencies": {
"@vitejs/plugin-react": "^4.1.0",
"vite-plugin-svgr": "^4.1.0",
"eslint-plugin-react-hooks": "^4.6.0",

Unused dependency

Unused dependency
maxime.batista marked this conversation as resolved
samuel.berion approved these changes 1 year ago
maxime.batista added 1 commit 1 year ago
continuous-integration/drone/push Build is failing Details
ed2cfa2013
apply suggestions
maxime.batista requested review from clement.freville2 1 year ago
maxime.batista force-pushed editor/arrows from ed2cfa2013 to ec46fb78a2 1 year ago
clement.freville2 requested changes 1 year ago
clement.freville2 left a comment

I saw you committed a sql/.guard file 👀

I saw you committed a `sql/.guard` file 👀
// If the (original) segments changes, overwrite the current ones.
useLayoutEffect(() => {
setInternalSegments(computeInternalSegments(segments))
}, [startPos, segments])
-     }, [startPos, segments]) 
+     }, [startPos, segments, computeInternalSegments])

This dependency always changes, so memorize it.

```diff - }, [startPos, segments]) + }, [startPos, segments, computeInternalSegments]) ``` This dependency always changes, so memorize it.
Poster

didn't added it because it causes an infinite recursion

didn't added it because it causes an infinite recursion

Yes, it recurses infinitely. That's why memoization is useful here. Wrap computeInternalSegments in a useCallback hook.

const computeInternalSegments = useCallback(
    (segments: Segment[]): FullSegment[] =>
        segments.map((segment, idx) => {
            // ...
        }),
    [startPos],
)
Yes, it recurses infinitely. That's why memoization is useful here. Wrap `computeInternalSegments` in a `useCallback` hook. ```ts const computeInternalSegments = useCallback( (segments: Segment[]): FullSegment[] => segments.map((segment, idx) => { // ... }), [startPos], ) ```
maxime.batista marked this conversation as resolved
startRadius,
endRadius,
style,
])
-     }, [
-         startPos,
-         internalSegments,
-         forceStraight,
-         startRadius,
-         endRadius,
-         style,
-     ])
+     }, [area, internalSegments, startPos, forceStraight, startRadius, endRadius, wavy])
```diff - }, [ - startPos, - internalSegments, - forceStraight, - startRadius, - endRadius, - style, - ]) + }, [area, internalSegments, startPos, forceStraight, startRadius, endRadius, wavy]) ```
maxime.batista marked this conversation as resolved
}, [update, containerRef])
// Inserts a segment where the mouse double clicks on the arrow
useEffect(() => {

So... Why not using a React event handler?

const addSegment = useCallback((e: MouseEvent) => {
  if (forceStraight) return
  // ...
}, [area, forceStraight, onSegmentsChanges, segments, startPos]);
// ...
return (
  <path onDoubleClick={addSegment} />
)
So... Why not using a React event handler? ```ts const addSegment = useCallback((e: MouseEvent) => { if (forceStraight) return // ... }, [area, forceStraight, onSegmentsChanges, segments, startPos]); // ... return ( <path onDoubleClick={addSegment} /> ) ```
maxime.batista marked this conversation as resolved
pathRef?.current?.addEventListener("dblclick", addSegment)
return () => {
pathRef?.current?.removeEventListener("dblclick", addSegment)

This is highly unsafe. The reference might have changed if the component has been re-rendered. Place the current reference in a temporary.

const current = pathRef.current!
current.addEventListener("dblclick", addSegment)
return () => current.removeEventListener("dblclick", addSegment)
This is highly unsafe. The reference might have changed if the component has been re-rendered. Place the current reference in a temporary. ```ts const current = pathRef.current! current.addEventListener("dblclick", addSegment) return () => current.removeEventListener("dblclick", addSegment) ```
maxime.batista marked this conversation as resolved
* A player that is placed on the court, which can be selected, and moved in the associated bounds
* */
export default function CourtPlayer({
export default function CourtPlayer<A extends ReactNode>({
- export default function CourtPlayer<A extends ReactNode>({
+ export default function CourtPlayer({
```diff - export default function CourtPlayer<A extends ReactNode>({ + export default function CourtPlayer({ ```
maxime.batista marked this conversation as resolved
role: player.role,
hasBall: player.hasBall,
})
} as Player)
-                 } as Player) 
+                 })
```diff - } as Player) + }) ```
maxime.batista marked this conversation as resolved
maxime.batista added 1 commit 1 year ago
continuous-integration/drone/push Build is failing Details
2ffca5a188
apply suggestions
maxime.batista requested review from clement.freville2 1 year ago
clement.freville2 approved these changes 1 year ago
clement.freville2 left a comment

The result looks great! Don't forget memoization and you are good to go.

The result looks great! Don't forget [memoization](https://codefirst.iut.uca.fr/git/IQBall/Application-Web/pulls/82#issuecomment-44541) and you are good to go.
maxime.batista added 1 commit 1 year ago
continuous-integration/drone/push Build is failing Details
1e6978be6e
apply suggestions
maxime.batista merged commit 368acee92e into master 1 year ago
maxime.batista deleted branch editor/arrows 1 year ago

Reviewers

yanis.dahmane-bounoua was requested for review 1 year ago
mael.daim was requested for review 1 year ago
vivien.dufour approved these changes 1 year ago
samuel.berion approved these changes 1 year ago
clement.freville2 approved these changes 1 year ago
continuous-integration/drone/push Build is failing
The pull request has been merged as 368acee92e.
Sign in to join this conversation.
No Milestone
No project
No Assignees
4 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: IQBall/Application-Web#82
Loading…
There is no content yet.