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

cube grid #92

Merged
merged 1 commit into from
May 27, 2023
Merged

cube grid #92

merged 1 commit into from
May 27, 2023

Conversation

gordonwoodhull
Copy link
Contributor

@gordonwoodhull gordonwoodhull commented May 26, 2023

Hi @gereleth!

Here is a draft of the cube grid (a step toward #83 P3 tiles). I think I've addressed everything we talked about, but if I missed anything or there's anything else you'd like changed, let me know.

I wasn't completely happy about duplicating the edge mark state for opposing tiles, but it's a really nice effect.

image

Moving all the transformation stuff into TransformedPolygonTile cleaned it up nicely. The only thing that is kind of weird is that all the method overrides have different arguments. The reason for this is mostly that the transforms need tile-relative but unflipped coordinates, so maybe this could be cleaned up.

@vercel
Copy link

vercel bot commented May 26, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
hexapipes ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 26, 2023 8:40pm

Copy link
Owner

@gereleth gereleth left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, I left some feedback.

Main points:

  • bug: click to orient only works in cubes, not anywhere else.
  • question: could we do the same edgemark visuals without adding reflectEdgeMarks to game state?

@@ -15,6 +15,7 @@ import { createViewBox } from './viewbox';
* @property {String} color
* @property {Boolean} locked
* @property {EdgeMark[]} edgeMarks
* @property {EdgeMark[]} reflectEdgeMarks
Copy link
Owner

Choose a reason for hiding this comment

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

Question: why do reflectEdgeMarks have to be a piece of game state? It seems to me that any info they provide we can already get from regular edgemarks with some neighbour logic. The end result looks great but why make game state heavier?..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am new to Svelte and thought that I needed to drive the marks from state. But now that I think about it, I can generate these on the fly by looking at state outside the current tile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the the edge marks are pulling part of the state from neighbours, I force the neighbour to redraw in game.toggleEdgeMark:

		if (self.grid.EDGEMARK_REFLECTS) {
			// force redraw of the neighbour because drawing of connection edgemarks
			// is driven partly by neighbours' state
			const neighbourState = self.tileStates[neighbour];
			neighbourState.set(neighbourState.data);
		}

Otherwise half-marks get left behind. I don't know if there is a better way to do this.

src/lib/puzzle/grids/cubegrid.js Outdated Show resolved Hide resolved
src/lib/puzzle/grids/cubegrid.js Outdated Show resolved Hide resolved
src/lib/puzzle/grids/cubegrid.js Outdated Show resolved Hide resolved
src/lib/puzzle/grids/cubegrid.js Outdated Show resolved Hide resolved
src/lib/puzzle/grids/hexagrid.js Outdated Show resolved Hide resolved
src/lib/puzzleWrapper/PuzzleInstanceWrapper.svelte Outdated Show resolved Hide resolved
with PR feedback addressed
and more consistent TransformedPolygonTile interface
@gordonwoodhull
Copy link
Contributor Author

gordonwoodhull commented May 26, 2023

Okay, I think I have addressed everything you suggested.

I also made TransformedPolygonTile simpler / more consistent by standardizing on tile coordinates for its inputs (whereas the RegularPolygonTile expects polygon coordinates).

The transformation framework should be all ready for P3 and whatever else. 🪄

Let me know if it needs any more changes. I have maintained open source and I know it's important to get everything right.

@gereleth
Copy link
Owner

Thanks for the changes!
I have more nitpicks... But I'm fine with merging this into a branch and finishing it myself.
I mean things like:

  • click to orient works with a mouse but not with a touch screen
  • starting viewbox of a large cube puzzle shows only empty space on phone screen
  • default scale of tiles is too small compared to other puzzles (because it's scaled for hexagons and rhombs are smaller)
  • "Play" header item is not highlighted on cube page

So if you'd rather start thinking about P3 and not deal with this then just say so)).

@gordonwoodhull
Copy link
Contributor Author

gordonwoodhull commented May 27, 2023

The mark gestures feel a little bit off due to the transformation. I have to remember to go with the false perspective and along the pipe/mark directions, instead of from center to center.

This is workable, but if I forget then nothing happens or the gesture can even cause the opposite effect, drawing a wall when I meant to draw a connection.

@gordonwoodhull
Copy link
Contributor Author

I owe it to myself to take a couple days off the computer (although I’m cheating this afternoon and playing cube wrap on my tablet).

So why don’t I take you up on your offer. Thank you!

I will catch up with your changes next week, as I do want to learn these details. (I have not seen the phone issue on iPhone or Android tablet & agree initial scale is too small, was unaware of the other issues.)

@gereleth gereleth changed the base branch from main to cubes May 27, 2023 19:56
@gereleth gereleth merged commit 4b68050 into gereleth:cubes May 27, 2023
@gordonwoodhull
Copy link
Contributor Author

🎉

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