Closed ghost closed 8 years ago
I like the code refactor, but I'm not sure about forcing the user to pick "one special piece" in move 1. Players should be able to pick any piece - this will also avoid confusion as there are no visual clues indicating that the separately placed piece should be picked on first move.
Have you tried it out? The visual clue is that the first piece is placed on the opponent's side of the board.
It seems pretty clear to me... but, of course, I implemented it so I know what to expect. Maybe it wouldn't be obvious to other players who are used to the original way of doing it. It always seemed strange to me that you "reach" across the board to pick up an opponent's piece.
We could also 'highlight' that first piece, though I thought it was unnecessary... again perhaps because I knew what to expect. If we did that it might be very nice, since it would give a visual clue as to whose move it is for the first two moves of the game.
I'm glad you like the refactor. I was a little nervous about it as the rabbit hole kept getting deeper.
Yes, I tried it out. That isn't much of a visual clue - it just looks like an oddly placed piece. We shouldn't expect a new player to click on every stack to figure out which piece to pick. Also, if we force a user to pick the piece on first move, we should force him to pick a piece on every move as would seem natural - which isn't really a good thing.
I'm not a fan of highlighting. The UI should be self explanatory with minimal constraints. If we go the route of highlighting, we've to highlight the pieces on every move as it'd be expected.
OK. I get it.
Would you like me to remove that 'first piece' stuff and resubmit the pull request for just the refactoring and other fixes?
That'd be great! I'd a question about the makeLetter function. Isn't passing index, side as parameters redundant as makeLetter is called for all sides and never for one specific side? I thought lettering/number for all sides can be put in that function. And since we use the loaded font only for lettering/numbering, we could just do that in the function itself to avoid passing it as parameter.
Yea, I wasn't sure about the makeLetter function either, especially after the switch to the TextureLoader.
I'll change that at the same time.
On Mon, May 30, 2016 at 12:15 AM, Chaitanya Vadrevu < notifications@github.com> wrote:
That'd be great! I'd a question about the makeLetter function. Isn't passing index, side as parameters redundant as we makeLetter is called for all sides and never for one specific side? I thought lettering/number for all sides can be put in that function. And since we use the loaded font only for lettering/numbering, we could just do that in the function itself to avoid passing it as parameter.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/chaitu236/TakWeb/pull/48#issuecomment-222418794, or mute the thread https://github.com/notifications/unsubscribe/ABVLPpK8WG_MM_MV28TuvZWz3bNDuIr8ks5qGoBlgaJpZM4IpBmV .
Great thanks!
I suppose the same thing could be said about the borders. We never make just one of the borders.
Ah yes.. didn't notice that
Just to be clear... I meant 'FontLoader' in that previous response (not TextureLoader).
Ah... I realize why I did it this way. It separates the concerns. The Factory knows how to make and position the "thing" being created (square, border, letter), and the caller adds it to the scene.
Regardless, I'll come up with something different... perhaps pass the scene to the factory. Let me know if you have a better idea.
boardFactory.makeSquare(i, j, scene); <<<< creates the square and adds it to the scene
boardFactory.makeBorders(scene); <<<< creates all 4 borders with their letters and adds them to the scene
Ok, now I realize the "factories" make more sense in a setting where the thing being created needs to be created alone. But in our case, all the squares, borders, letters/numbers, pieces/capstones are created together, so just separate functions for each of these categories would suffice.
You must have spent a lot of time on this refactor - the resulting code looks cleaner and I don't want to throw it out - but it also results in unnecessary abstraction and inefficiencies - so I am confused what to do.
Yea, I agree with your observation of the factories.
See what you think of my latest change. It was pretty easy to make. Now I pass the scene and the factories add the objects being created to the scene. The methods return the object being added if the caller has need of it (e.g. squares and pieces). But the borders are not returned.
I suppose this is a reasonable middle ground. If you update the pull request with these changes I'll merge.
Lots of changes wrapped up in this:
Unrelated change: