Skip to content
This repository has been archived by the owner on Jul 7, 2022. It is now read-only.

Display a static chess state on AR #209

Merged
merged 24 commits into from
Apr 7, 2022
Merged

Display a static chess state on AR #209

merged 24 commits into from
Apr 7, 2022

Conversation

KurohanaJuri
Copy link
Contributor

@KurohanaJuri KurohanaJuri commented Apr 2, 2022

This PR change the AR display, we can now display a board with pieces with colour.

Big changes :

  • The models are loaded and added into the view at creation
    • This fix an old bug that makes impossible to use the function smooth to move an object from a point A to B

This PR also centralize the graphic content into one place.

Next step :

For the moment, the scene only display a static board. We need some modification to be able to update the AR game with the incoming new state. We need to add a listener that will change one piece position to the new location. To do that, we have 2 possibility :

  • "Spawning" : We change the position without animation, the piece disappear to spawn on the new position
  • With animation : We can do a smooth moving animation. This solution is a bit more challenging if we don't want a "cheap" effect (Piece that collide with other). The best animation is a bell-shaped trajectory, which is tricky.

Possible Enhancement :

  • Allow user to customize the piece colour
    • The colour is set dynamically, we can imagine having multiple preset that the user can choose (or pay to have it ;D)
    • The board customization is a bit trickier, it's not a uniform colour, hence the "colour" is a texture come with the models, we can have multiple board skin in different files
  • We can also have another set of pieces
  • Display match information in AR (player turn, end of the game,....)

The state that is displayed is simply the board obtain after the call `Game.create().board`
This version not final, we need to bind this to a real `Match` and some refactoring is needed to
be readable
@KurohanaJuri KurohanaJuri self-assigned this Apr 2, 2022
@KurohanaJuri KurohanaJuri marked this pull request as draft April 2, 2022 18:45
- Create 2 new nested functions to make the code more pleasant to read :
	- `getModelPath`
	- `getModelColor`
- Remove some unnecessary variable
- Correct null check to be more kotlin like
…R chess

When the constructor is called, the class load the board and the peices
depending on the game state.
- At creation, the root node shouldn't be null
- When we scale the whole scene, the root node should have the correct
scale vector
Copy link
Contributor

@alexandrepiveteau alexandrepiveteau left a comment

Choose a reason for hiding this comment

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

Looks good, and it's great that the coverage is above 80%.

I feel like there's a small violation of the MVVM pattern though - the view should not use a Game directly, since it's a Model object; instead, I think the approach should be similar to the one used in the 2d chessboard. I was thinking we may make a refactoring of the ChessBoardState in the next sprint so we can extract the common logic for loading / managing the game state between the 2d and 3d boards. What are your thoughts on this ?

@KurohanaJuri KurohanaJuri enabled auto-merge (squash) April 7, 2022 07:57
@matt989253
Copy link
Contributor

A few things to change, but looks pretty good! It's awesome that you managed to bring the coverage above 80%!

KurohanaJuri and others added 5 commits April 7, 2022 15:59
…sSceneTest.kt

Co-authored-by: Matthieu Burguburu <36692720+matt989253@users.noreply.github.com>
It used to distinguish between engine position and Ar position
@codeclimate
Copy link

codeclimate bot commented Apr 7, 2022

Code Climate has analyzed commit 0650f5c and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 81.1% (80% is the threshold).

This pull request will bring the total coverage in the repository to 96.8% (1.3% change).

View more on Code Climate.

Copy link
Contributor

@matt989253 matt989253 left a comment

Choose a reason for hiding this comment

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

LGTM!

@KurohanaJuri KurohanaJuri merged commit 8077a31 into main Apr 7, 2022
@KurohanaJuri KurohanaJuri deleted the feature/cyk/ar_match branch April 7, 2022 16:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants