-
-
Notifications
You must be signed in to change notification settings - Fork 377
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
Simplify split function #411
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General approval of this change with suggestions that are non blocking.
I think if we're going to do this, then what about if we go one step further and remove Element altogether?
The only variables that we care about are the end
of each chunk (as the start
is required to be strictly equal to the previous chunk).
I think there's probably even a simple approach where the start and end of the area don't really need to be treated any differently than the start / end of a chunk (using an Vec of the positions and .window(2)
to setup every constraint).
A quick and dirty check of this suggests that we delete 18-24 lines of code in the diff
Note - this doesn't have #408 included yet (just merged that) |
f3bbcdb
to
7693ec2
Compare
Codecov Report
@@ Coverage Diff @@
## main #411 +/- ##
==========================================
- Coverage 86.13% 86.13% -0.01%
==========================================
Files 40 40
Lines 9240 9239 -1
==========================================
- Hits 7959 7958 -1
Misses 1281 1281
|
I'm not sure if I'm following this suggestion. We'll always need two decision variables for each chunk, e.g. Also for consideration, we might want to add this constraint in the future: for i in 0..elements.len() {
for j in (i + 1)..elements.len() {
solver.add_constraint(elements[j].size() | EQ(WEAK) | (elements[i].size()))?;
}
} Regardless of a possible simplification, I do like that |
7693ec2
to
2247ed7
Compare
I'm going to go ahead and merge this for now, because I'm not sure how much time I'll have in front of a computer over the weekend. I'm happy to follow up on the further simplification in a separate PR. |
If you have: let (area_start, area_end, area_size) = match layout.direction {
Direction::Horizontal => (
f64::from(inner.left()),
f64::from(inner.right()),
f64::from(inner.width),
),
Direction::Vertical => (
f64::from(inner.top()),
f64::from(inner.bottom()),
f64::from(inner.height),
),
};
// the positions of each chunk. The first variable is the start, and each successive variable is
// the end of that chunk and the start of the next one
let positions = std::iter::from_fn(|| Some(Variable::new()))
.take(layout.constraints.len() + 1)
.collect_vec();
// ensure the first element is at the start
solver.add_constraint(positions[0] | EQ(REQUIRED) | area_start)?;
// ensure the positions are monotonically increasing
for &position in positions.iter() {
solver.add_constraints(&[
position | GE(REQUIRED) | area_start,
position | LE(REQUIRED) | area_end,
])?;
}
// ensure the last element touches the right/bottom edge of the area
if layout.expand_to_fill {
if let Some(&last) = positions.last() {
solver.add_constraint(last | EQ(REQUIRED) | area_end)?;
}
}
let sizes = positions
.iter()
.tuple_windows()
.map(|(&start, &end)| end - start)
.collect_vec();
// apply the constraints
for (size, &constraint) in sizes.iter().cloned().zip(layout.constraints.iter()) {
solver.add_constraint(size.clone() | GE(REQUIRED) | 0.0)?;
match constraint {
Constraint::Percentage(p) => {
let percent = f64::from(p) / 100.00;
solver.add_constraint(size | EQ(STRONG) | (area_size * percent))?;
}
Constraint::Ratio(n, d) => {
// avoid division by zero by using 1 when denominator is 0
let ratio = f64::from(n) / f64::from(d.max(1));
solver.add_constraint(size | EQ(STRONG) | (area_size * ratio))?;
}
Constraint::Length(l) => solver.add_constraint(size | EQ(STRONG) | f64::from(l))?,
Constraint::Max(m) => {
solver.add_constraints(&[
size.clone() | LE(STRONG) | f64::from(m),
size | EQ(WEAK) | f64::from(m),
])?;
}
Constraint::Min(m) => {
solver.add_constraints(&[
size.clone() | GE(STRONG) | f64::from(m),
size | EQ(WEAK) | f64::from(m),
])?;
}
}
}
This can be written as: for (left, right) in sizes.iter().tuple_window() {
solver.add_constraint(left | EQ(WEAK) | right)?;
} It's probably not necessary to add all the comparisons as [a=b, b=c] implies [a=c]. I'm not sure what the constraint solver does though for constraints that can't be satisfied. |
Okay, that makes sense. Thanks for explaining and the code examples. I do find it more confusing to read though, but maybe I should think about it more.
I think it is necessary to add all combinations because you can have one of the chunks be smaller because of a higher priority constraint. For example take a case like |
You're right. I tried that and found that I was wrong. Your explanation of why makes a lot of sense - I couldn't quite get there. I replaced Percentage shouldn't grow / shrink any more than length should, as it's based on a percentage of the total not a percentage of the amount after length is taken out. 50%/10/50% should render (these would be good tests in the other PR):
I thought the same when I initially read it from the perspective of coming from thinking in terms of
|
A diagram like this certainly helps:
I'm not convinced it is worth it for the extra simplification in this case. If we really wanted to remove But I do like the way it is written right now with |
Further simplification of
try_split
by removing thearea
element and constraints for that, and using thestart
andend
values directly.