Use f64 instead of usize for fragment widths
#421
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mgeisler
force-pushed
the
f64-fragment-widths
branch
2 times, most recently
from
6948a3d
to
023bb76
Compare
mgeisler
added a commit
that referenced
this pull request
Jan 9, 2022
The optimization problem solved by the optimal-fit algorithm is fundamentally a minimization problem. It is therefore not sensible to allow negative penalties since all penalties are there to discourage certain features: * `nline_penalty` discourages breaks with more lines than necessary, * `overflow_penalty` discourages lines longer than the line width, * `short_last_line_penalty` discourages short last lines, * `hyphen_penalty` discourages hyphenation Making this change surfaces the overflow bug behind #247 and #416. This will be fixed next via #421 and this commit can be seen as a way of simplifying that PR.
mgeisler
added a commit
that referenced
this pull request
Jan 9, 2022
The optimization problem solved by the optimal-fit algorithm is fundamentally a minimization problem. It is therefore not sensible to allow negative penalties since all penalties are there to discourage certain features: * `nline_penalty` discourages breaks with more lines than necessary, * `overflow_penalty` discourages lines longer than the line width, * `short_last_line_penalty` discourages short last lines, * `hyphen_penalty` discourages hyphenation Making this change surfaces the overflow bug behind #247 and #416. This will be fixed next via #421 and this commit can be seen as a way of simplifying that PR.
mgeisler
added a commit
that referenced
this pull request
Jan 9, 2022
The optimization problem solved by the optimal-fit algorithm is fundamentally a minimization problem. It is therefore not sensible to allow negative penalties since all penalties are there to discourage certain features: * `nline_penalty` discourages breaks with more lines than necessary, * `overflow_penalty` discourages lines longer than the line width, * `short_last_line_penalty` discourages short last lines, * `hyphen_penalty` discourages hyphenation Making this change surfaces the overflow bug behind #247 and #416. This will be fixed next via #421 and this commit can be seen as a way of simplifying that PR.
mgeisler
added a commit
that referenced
this pull request
Jan 9, 2022
The optimization problem solved by the optimal-fit algorithm is fundamentally a minimization problem. It is therefore not sensible to allow negative penalties since all penalties are there to discourage certain features: * `nline_penalty` discourages breaks with more lines than necessary, * `overflow_penalty` discourages lines longer than the line width, * `short_last_line_penalty` discourages short last lines, * `hyphen_penalty` discourages hyphenation Making this change surfaces the overflow bug behind #247 and #416. This will be fixed next via #421 and this commit can be seen as a way of simplifying that PR.
mgeisler
force-pushed
the
f64-fragment-widths
branch
2 times, most recently
from
dbcc245
to
432f20d
Compare
This changes the type used for internal width computations in the wrap
algorithms. Before, we used `usize` to represent the fragment widths
and for the line widths. This could make the optimal-fit wrapping
algorithm overflow when it tries to compute the optimal wrapping cost.
The problem is that the algorithm computes a cost using integer values
formed by
(line_width - target_width)**2
When `line_width` is near `usize::MAX`, this computation can easily
overflow.
By using an `f64` for the cost computation, we achieve two things:
* A much larger range for the cost computation: `f64::MAX` is about
1.8e308 whereas `u64::MAX` is only 1.8e19. Computing the cost with a
fragment width in the range of `u64`, will thus not exceed 3e38,
something which is easily represented with a `f64`. This means that
wrapping fragments derived from a `&str` cannot overflow.
Overflows can still be triggered when fragments with extreme
proportions are formed directly. The boundary seems to be around
1e170 with fragment widths above this limit triggering overflows.
* Applications which wrap text using proportional fonts will already
be operating with widths measured in floating point units. Using
such units internally makes life easier for such applications, as
shown by the changes in the Wasm demo.
Fixes #247
Fixes #416
This tests the wrapping using fragments with widths which could come from a &str.
This changes the panics in `wrap_optimal_fit` to a `Result` type, allowing clients to catch them.
mgeisler
force-pushed
the
f64-fragment-widths
branch
from
432f20d
to
d380b95
Compare
This was referenced Jan 9, 2022
mgeisler
added a commit
that referenced
this pull request
Feb 5, 2022
Following the advice from [1], this PR updates Cargo.toml to use precise version numbers for all dependencies. The latest versions at the time of writing are used. It turns out that we made precisely the mistake mentioned in the post: ``` % cargo -Z minimal-versions update % cargo test ``` failed on nightly because of the dependency on smawk 0.3: we need 0.3.1 after #421. [1]: https://users.rust-lang.org/t/psa-please-specify-precise-dependency-versions-in-cargo-toml/71277
mgeisler
added a commit
that referenced
this pull request
Feb 5, 2022
Following the advice from [1], this PR updates Cargo.toml to use precise version numbers for all dependencies. The latest versions at the time of writing are used. It turns out that we made precisely the mistake mentioned in the post: ``` % cargo -Z minimal-versions update % cargo test ``` failed on nightly because of the dependency on smawk 0.3: we need 0.3.1 after #421. [1]: https://users.rust-lang.org/t/psa-please-specify-precise-dependency-versions-in-cargo-toml/71277
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This changes the type used for internal width computations in the wrap algorithms. Before, we used
usizeto represent the fragment widths and for the line widths. This could make the optimal-fit wrapping algorithm overflow when it tries to compute the optimal wrapping cost. The problem is that the algorithm computes a cost using integer values formed byWhen
line_widthis nearusize::MAX, this computation can easily overflow.By using an
f64for the cost computation, we achieve two things:A much larger range for the cost computation:
f64::MAXis about 1.8e308 whereasu64::MAXis only 1.8e19. Computing the cost with a fragment width in the range ofu64, will thus not exceed 3e38, something which is easily represented with af64. This means that wrapping fragments derived from a&strcannot overflow.Overflows can still be triggered when fragments with extreme proportions are formed directly. The boundary seems to be around 1e170 with fragment widths above this limit triggering overflows.
Applications which wrap text using proportional fonts will already be operating with widths measured in floating point units. Using such units internally makes life easier for such applications, as shown by the changes in the Wasm demo.
Fixes #247
Fixes #416