Add comparison constraints #7
Conversation
|
Thanks for looking at this. I've been wondering whether to implement something like this as the thermo line display can be ambiguous when they overlap. A general note: Please add an example of the constraint you are adding. This makes it easier to check that it is doing the right thing, and that we are on the same page about what the constraint is. Some comments about the naming:
As an aside, adding many Comparison constraints looks like it would get tedious, and I'm wondering whether the UI should make that easier, such as allowing an arbitrarily long selection and adding it between any two consecutive adjacent cells. (This is relevant for kropki and XV constraints as well). |
|
min/max sound much better. I was considering the "create many" use case. I feel like there needs to a separate category for "cell constraints", that would also be able to handle parity, etc restrictions. I just didn't want to do anything like that without first verifying my understanding of basic design rules. There was another thing I was pondering was being able to make "orthogonal" and "all adjacent" constraint that just applied a single existing binary constraint to all orthogonal/adjacent cells, but I wasn't sure what the best way to do that without changing the current constraint construction path. I was also briefly considering adding the concept of "implicit cells" for the UI when selecting a constraint, but realized I was feature creeping a relational operator :D |
|
I would definitely welcome UI improvements that make things easier and remain intuitive. However, it's worth keeping any bigger changes out of this commit. Thinking about it a bit more, I'm on the fence about adding min/max, just for the reason that the list of constraints in the "Lines & Sets" dropdown is quite long and these would be redundant constraints.
The easiest way without changing any of the constraint construction logic is to either:
A third option is to do this splitting during constraint construction, which isn't too hard as it would be completely within
My main concern is with cluttering up the UI more, both from the amount of space it takes up and with forcing users to hunt through different menus to find what they need. I agree that there are quite a few disparate constraints in the "Lines & Sets" section. I'd suggest checking in GreaterThan, before worrying about any UI improvements, as any generalisation from the 2-cell case should be backward compatible. |
Mangled my local git history, but this is the result of adding fortress constraints, and then removing them, and addressing some naming.
ojhunt
force-pushed
the
upstream/dev/comparisons
branch
from
409c808 to
d1ab0e8
Compare
|
Clobbered my git history due to local nonsenses. I've gone for allowing multiple cells as long as they're all orthogonally connected to the first one (rather than finding a central cell) and relabelled to max. I've removed the change in rendering style for one vs many comparisons, and removed the attempt to unique binary constraints such that a < b is unified with b > a. Not really sure what the best approach to that problem is at the moment (the general solution requires searching for neighboring overlaps rather than simply relying on a uniqueness hash). |
| @@ -18,6 +18,7 @@ class ConstraintDisplays { | |||
| this.Quad, | |||
| this.Givens, | |||
| this.OutsideClue, | |||
| this.Comparison, | |||
There was a problem hiding this comment.
Move this before Letter. Constraints are rendered in this order.
| 'L', x - comparisonDirection * comparisonSize * squash - dC * inset, y + comparisonSize, | ||
| ] | ||
| } | ||
| let path = createSvgElement('path'); |
There was a problem hiding this comment.
Use this._makePath, then you can just pass a list of points.
| const secondaryCells = constraint.secondaryCells; | ||
|
|
||
| let result = createSvgElement("g"); | ||
| let drawDecoration = (primaryCell, secondaryCell) => { |
There was a problem hiding this comment.
Pull this out into a proper method. No reason for this to be inline.
| const primaryCell = constraint.primaryCell; | ||
| const secondaryCells = constraint.secondaryCells; | ||
|
|
||
| let result = createSvgElement("g"); |
| const comparisonSize = 0.1 * cellSize; | ||
| const inset = 0; | ||
| const squash = 0.75; | ||
| var dC = x0 < x1 ? 1 : (x0 > x1 ? -1 : 0); |
| const comparisonSize = 0.1 * cellSize; | ||
| const inset = 0; | ||
| const squash = 0.75; | ||
| var dC = x0 < x1 ? 1 : (x0 > x1 ? -1 : 0); |
There was a problem hiding this comment.
const dC = x1-x0; should be equivalent because the cells are adjacent.
There was a problem hiding this comment.
Oh, sorry, forgot this wasn't an index. Then just use Math.sign()
| @@ -1632,6 +1632,63 @@ class SudokuConstraint { | |||
| ); | |||
| } | |||
|
|
|||
| static Comparison = class Comparison extends SudokuConstraintBase { | |||
There was a problem hiding this comment.
As discussed, change to GreaterThan
This is just a real basic pair of comparison constraints, a simple binary comparison, and what I think are called fortress cells. Mostly I want feedback/suggestions on what I've done so far.
They share the display logic, with some procedural logic to match what seems consistent with what people seem to use. The display logic just distinguishes rendering policy on the basis of 1 vs >1 comparison which "works" for the current behavior, and does match the rendering of general </> constraints and multiple secondary fortress cells.
The fortress cells currently implicitly collects orthogonal cells rather than requiring explicit selection of secondary cells, but there are puzzles that do a subset of orthogonal cells, and even just a binary constraint gets the same rendering as a cell with multiple, and yet other comparison cases where the cells have comparisons in different directions.