-
Notifications
You must be signed in to change notification settings - Fork 17
Description
Updating {systemfonts} from 1.2.3 to 1.3.1 breaks our {ggforce} (currently at 0.5.0), e.g. segfaulting this example:
The entrypoint into {systemfonts} from {ggforce} is thus:
systemfonts::font_info("", logical(), logical(), 12, res = 300)It looks like the update to recycle vectors in #138 is related, because rep_len(logical(), 1L) becomes NA, which doesn't appear handled, e.g. here passes the NA integer value on to other libraries' code:
systemfonts/src/font_matching.cpp
Line 233 in beb28a8
| locate_systemfont( |
systemfonts/src/font_matching.cpp
Line 60 in beb28a8
| key.italic = italic; |
I threw some agents at this and they came up with two different solutions -- I don't know enough about the package to judge whether either is better, or if something else is off and another approach would be preferred.
- Check
NAfrom some helpers in FontDescriptor.h, e.g.
+#include <Rmath.h>
inline int italic_to_fixed(double italic) {
+ if (ISNA(italic)) return 0;
return italic < 0.0 ? 0 : (italic > 1.0 ? FIXED_MOD : italic * FIXED_MOD);
}- Check length-0 inputs from R and replace them with defaults, e.g.
Line 87 in beb28a8
| italic <- rep_len(italic, full_length) |
+ if (!length(italic)) italic <- FALSE
italic <- rep_len(italic, full_length)
weight <- rep_len(weight, full_length)
width <- rep_len(width, full_length)(happy to share either as a PR if you'd like)
I also couldn't figure out why this is an issue for us and not CRAN, probably some version mismatch (we are using R 4.5.0, FreeType 2.13.3, and fontconfig 2.16.2) or a stricter compiler. I did notice an error (not a crash) from the same lines on a {ggforce} PR: https://github.com/thomasp85/ggforce/actions/runs/19674040486/job/56350141145?pr=352.
It's possible this is WAI but it looked off to me to be possibly passing NA to a native library.