-
Notifications
You must be signed in to change notification settings - Fork 104
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
Fix crash related to not found glyphs. #1789
Conversation
spring/rts/Rendering/Fonts/CFontTexture.h Line 186 in a2dd682
This may resolve to a spring/rts/System/UnorderedMap.hpp Lines 25 to 26 in a2dd682
Using So we shouldn't use if (iter != glyphs.end()) {
const auto [insertee, success] = glyphs.emplace(ch, iter->second);
assert(success);
insertee->letter = ch; // the PR removes this, but i assume `letter` is about the actual shape so could matter for kerning or whatnot so probably good to keep. idk.
return;
} I guess that's what the current state of the PR does under the hood already, though without the spooky |
@p2004a seems to be good at C++ STL so maybe he could also take a look. |
The shape is actually determined by the insertee->face and the insertee->index attribute. The code here is inserting different chars that resolve to the same glyph, which was stored before with a certain letter as key and the same letter inside the glyph. Changing the letter here just makes us store more kerning hashes later on and recalculate kerning, but has no other effect imo since at the end of the day it's going to be the same glyph and I think it should have the same kerning. It does look a bit awkward and could be put back into the PR but imo it's more optimal this way. Maybe instead of letter could be called glyph.letter is only used here: spring/rts/Rendering/Fonts/CFontTexture.cpp Line 689 in 07dd4bc
Anyways since that's not really relevant to the main issue the PR is trying to fix, I can put it back if you feel this is overextending. |
Thanks for shedding some light into this :D Maybe the issue with the [] operator is that it might not be a std::unordered_map like you say, then I don't know what'll happen XD. If it's an unordered_map, it should be fine, according to docs: "If after the operation the new number of elements is greater than old max_load_factor() * bucket_count() a rehashing takes place. So (I believe) it should be totally safe to do What I like about Don't have a problem in changing to |
Yes, that old code was bad, but the new one looks a bit suspicious too.
|
btw it might be a good idea to rewrite
into
or even generalize this kind of construct into
|
Yes was thinking the same. so it's easier to share glyphs among different chars. Anyways before big rewrites I think it's good to think about it more. Tbh the glyph sharing is pretty niche since usually with fontconfig you will be finding glyphs for every char. For now anyways I think it's good to fix this with something simple, will keep that construct in mind for later. |
Done, imo it's not needed at least when using std::unordered_map but ok. |
Work done
Remarks
Note this all happens with current standard engine, also with custom compiled without any of the recent fontconfig changes, also fontconfig here doesn't take part since I'm using
UseFontConfigLib = 0
anyways.found this while testing the engine with bar and option
UseFontConfigLib = 0
and some font that doesn't have alphabet glyphs inside BAR/fonts.also removed assigning chr to the GlyphInfo (
glyph.letter = ch
) since it already has a previous character so no reason to change it here. We look it up in the std::pair->first anyways, and changing the char here can make it not share some other caches with other characters sharing the same glyph (rn the kerninginfo).Was trying
UseFontConfigLib = 0
as a possible workaround for the Zorin Os player, and decided to investigate the crash since it might be affecting other players too. Funnily the workaround pbbly can't be used by the player since afaik when playing with launcher it will overwrite the springsettings.cfg config all the time.Crash debug
The affected code:
spring/rts/Rendering/Fonts/CFontTexture.cpp
Lines 860 to 864 in 07dd4bc
Tbh not entirely sure why it crashes XD. I know but don't know the ultimate reasons.
I investigated, and seems like sometimes (it's only after a few times entering the loop, not sure why, maybe 20) the assignment operator will get an empty GlyphInfo for some reason when calling
glyph = iter->second;
.A dirtier workaround seems to be doing some
glyphs[ch]
access before the ``return`, in that case it all works!! Might be something with the compiler? Corruption before this?Sorry that I don't have a more final analysis :-P
Logs
These are custom logs, interpret like:
glyph-dupe-ok.txt
glyph-dupe-crash.txt (just printing the chain that lead to the crash, there were more dupe glyphs before this)
The crash happens after
[t=00:00:12.223282][f=-000001] [Font] DUPE GLYPH Noto Sans Symbols2 ['] 0 0x5b0d599a93c0 / 0 [~] 0x5b0dae3dcea0 21 0.588235
.That means the iter->second.face has 21 count on the smart pointer.
The logs also show
iter->second
is ok, also everything is fine afterauto& glyph = glyphs[ch];
, but then when doingauto& glyph = glyphs[ch];
the assignment operator gets an uninitialized 'other' and crashes (or that's my interpretation).They logs are printed roughly with this code: