Fix broken string wrapping and language loading#19391
Merged
Merged
Conversation
ZehMatt
suggested changes
Feb 13, 2023
ZehMatt
left a comment
Contributor
There was a problem hiding this comment.
Some minor things otherwise seems fine.
e9b4bcd to
9fdfe83
Compare
ZehMatt
reviewed
Feb 13, 2023
| */ | ||
| int32_t GfxGetStringWidthNewLined(std::string_view text, FontStyle fontStyle) | ||
| { | ||
| thread_local std::string buffer; |
Contributor
There was a problem hiding this comment.
I believe this was done to avoid allocations as it acts only as a temporary buffer. But I think I can let this slip until it shows up during a random profiling session again.
Contributor
Author
There was a problem hiding this comment.
Since this string gets resized to 0 anyways I'm not that convinced it saves any allocation time - as that 0 resize may be freeing the memory.
Contributor
There was a problem hiding this comment.
resize/clear actually keeps the capacity, it won't destroy the buffer, you have to use https://en.cppreference.com/w/cpp/string/basic_string/shrink_to_fit in order to free unused capacity.
Contributor
Author
There was a problem hiding this comment.
I see, regardless if this is an issue in the future it may be a good idea to roll out a frame allocator/stack based string solution for such temporaries. A std::string-like type with a 256-512b big small string optimization space on the stack should cover 99.9% of the hot code paths. It's done this way e.g. in DuckStation, with success.
Contributor
There was a problem hiding this comment.
I've been toying with the idea of having scratch buffers that are cleared after every frame is fully processed, however I would like to first have more code refurbished before adding more logic. At the moment this is not a noticeable issue so its not exactly high priority.
Contributor
|
Code looks good but I also want to give it a test and with ASAN compiled. |
ZehMatt
approved these changes
Feb 13, 2023
ZehMatt
left a comment
Contributor
There was a problem hiding this comment.
No more ASAN errors, can't say it has made the performance worse either so all good.
Member
|
Since this is a pretty long-standing issue, I think this warrants a changelog entry. |
Contributor
Author
|
@ZehMatt were you able to confirm that the draw summary crash is related? I specifically avoided linking them as this fix has nothing to do with formatting - unless the format function crashes due to an improperly wrapped string. |
The code interpreting fileData assumes the string has a valid null terminator, which is not guaranteed by std::vector, but is guaranteed by std::string.
Fixes a consistent assertion when displaying "Cannot start construction" in Japanese, but it has potential fix countless other crashes related to string wrapping/display.
9fdfe83 to
0763fd5
Compare
Contributor
It seems to fix it all together, I tried placing some coasters where they can't be build on Japanese and no crashes or hangs on my end, I'm guessing that one of the functions corrupted the data and passed it to the formatter. |
janisozaur
added a commit
that referenced
this pull request
Mar 28, 2023
- Feature: [#11269] Add properties for speed and length of vehicle animations. - Feature: [#15849] Objectives can now be set for up to 50000 guests. - Feature: [#18537] Add shift/control modifiers to window close buttons, closing all but the given window or all windows of the same type, respectively. - Feature: [#18732] [Plugin] API to get the guests thoughts. - Feature: [#18744] Cheat to allow using a regular path as a queue path. - Feature: [#19023] Add Canadian French translation. - Feature: [#19341] Add “All Scenery” tab to scenery window. - Feature: [#19378] Add command to combine CSG1i.DAT and CSG1.DAT. - Feature: [objects#226] Port RCT1 Corkscrew Coaster train. - Feature: [objects#229] Port RCT1 go karts with helmets. - Feature: [OpenMusic#20, OpenMusic#21] Added Blizzard and Extraterresterial ride music styles. - Improved: [#11473] Hot reload for plug-ins now works on macOS. - Improved: [#12466] RCT1 parks now use RCT1’s interest calculation algorithm. - Improved: [#14635] Scenery window now shows up to 255 scenery groups. - Improved: [#17288] Reorganise the order of shortcut keys in the Shortcut Keys window. - Improved: [#18706] Ability to view the list of contributors in-game. - Improved: [#18749] Ability to have 4 active awards for more than one month in a row. - Improved: [#18826] [Plugin] Added all actions and their documentation to plugin API. - Improved: [#18945] Languages can now fall back to other languages than English. - Improved: [#18970] Trying to load a non-park save will now display a context error. - Improved: [#18975] Add lift sprites for steep hills on the wooden roller coaster. - Improved: [#19044] Added special thanks to RMC and Wiegand to the About page. - Improved: [#19131] Track missing objects when selecting scenery groups in console. - Improved: [#19253] Queue junctions drawn properly when using regular paths as queue. Note: Requires using tile inspector to indicate railings can be used at T or X junctions. - Improved: [#19067] New Ride window now allows filtering similarly to Object Selection. - Improved: [#19272] Scenery window now allows filtering similarly to Object Selection. - Improved: [#19447] The control key now enables word jumping in text input fields. - Improved: [#19463] Added ‘W’ and ‘Y’ with circumflex to sprite font (for Welsh). - Improved: [#19549] Enable large address awareness for 32 bit Windows builds allowing to use 4 GiB of virtual memory. - Improved: [#19668] Decreased the minimum map size from 13 to 3. - Improved: [#19683] The delays for ride ratings to appear has been reduced drastically. - Improved: [#19697] “Show guest purchases” will now work in multiplayer. - Change: [#19018] Renamed actions to fit the naming scheme. - Change: [#19091] [Plugin] Add game action information to callback arguments of custom actions. - Change: [#19233] Reduce lift speed minimum and maximum values for “Classic Wooden Coaster”. - Removed: [#19520] Support for Windows Vista systems. - Fix: [#474] Mini golf window shows more players than there actually are (original bug). - Fix: [#592] Window scrollbar not able to navigate to the end of large lists. - Fix: [#7210] Land tile smoothing occurs with edge tiles (original bug). - Fix: [#17996] Finances window not cleared when starting some .park scenarios. - Fix: [#18260] Crash opening parks that have multiple tiles referencing the same banner entry. - Fix: [#18467] “Selected only” Object Selection filter is active in Track Designs Manager, and cannot be toggled. - Fix: [#18904] OpenRCT2 audio object accidentally exported in saves. - Fix: [#18905] Ride Construction window theme is not applied correctly. - Fix: [#18911] Mini Golf station does not draw correctly from all angles. - Fix: [#18971] New Game does not prompt for save before quitting. - Fix: [#18986] [Plugin] Sending remote scripts larger than 63KiB crashing all clients. - Fix: [#18994] Title music doesn’t start after enabling master volume. - Fix: [#19025] Park loan behaves inconsistently with non-round and out-of-bounds values. - Fix: [#19026] Park loan is clamped to a 32-bit integer. - Fix: [#19068] Guests may not join queues correctly. - Fix: [#19091] [Plugin] Remote plugins in multiplayer servers do not unload properly. - Fix: [#19112] Clearing the last character in the Object Selection filter does not properly reset it. - Fix: [#19112] Text boxes not updated with empty strings in Track List, Server List, and Start Server windows. - Fix: [#19114] [Plugin] ‘GameActionResult’ does not comply to API specification. - Fix: [#19136] SV6 saves with experimental RCT1 paths not imported correctly. - Fix: [#19243] .park scenarios don’t complete properly. - Fix: [#19250] MusicObjects do not free their preview images. - Fix: [#19292] Overflow in ‘totalRideValue’. - Fix: [#19339] Incorrect import of crashed particles from SV4. - Fix: [#19379] “No platforms” station style shows platforms on the Junior Roller Coaster. - Fix: [#19380] Startup crash when no sequences are installed and random sequences are enabled. - Fix: [#19391] String corruption caused by an improper buffer handling in ‘GfxWrapString’. - Fix: [#19434, #19509] Object types added by OpenRCT2 do not get removed when executing ‘remove_unused_objects’. - Fix: [#19475] Cannot increase loan when more than £1000 in debt. - Fix: [#19493] SV4 saves not importing the correct vehicle colours. - Fix: [#19517] Crash when peeps try to exit or enter hacked rides that have no waypoints specified. - Fix: [#19524] Staff counter shows incorrect values if there are more than 32767 staff members. - Fix: [#19574] Handle exits in null locations. - Fix: [#19641, #19643] Missing water tile in Infernal Views’ and Six Flags Holland’s river.
qwzhaox
pushed a commit
to qwzhaox/OpenRCT2
that referenced
this pull request
Apr 10, 2023
- Feature: [OpenRCT2#11269] Add properties for speed and length of vehicle animations. - Feature: [OpenRCT2#15849] Objectives can now be set for up to 50000 guests. - Feature: [OpenRCT2#18537] Add shift/control modifiers to window close buttons, closing all but the given window or all windows of the same type, respectively. - Feature: [OpenRCT2#18732] [Plugin] API to get the guests thoughts. - Feature: [OpenRCT2#18744] Cheat to allow using a regular path as a queue path. - Feature: [OpenRCT2#19023] Add Canadian French translation. - Feature: [OpenRCT2#19341] Add “All Scenery” tab to scenery window. - Feature: [OpenRCT2#19378] Add command to combine CSG1i.DAT and CSG1.DAT. - Feature: [objects#226] Port RCT1 Corkscrew Coaster train. - Feature: [objects#229] Port RCT1 go karts with helmets. - Feature: [OpenMusic#20, OpenMusic#21] Added Blizzard and Extraterresterial ride music styles. - Improved: [OpenRCT2#11473] Hot reload for plug-ins now works on macOS. - Improved: [OpenRCT2#12466] RCT1 parks now use RCT1’s interest calculation algorithm. - Improved: [OpenRCT2#14635] Scenery window now shows up to 255 scenery groups. - Improved: [OpenRCT2#17288] Reorganise the order of shortcut keys in the Shortcut Keys window. - Improved: [OpenRCT2#18706] Ability to view the list of contributors in-game. - Improved: [OpenRCT2#18749] Ability to have 4 active awards for more than one month in a row. - Improved: [OpenRCT2#18826] [Plugin] Added all actions and their documentation to plugin API. - Improved: [OpenRCT2#18945] Languages can now fall back to other languages than English. - Improved: [OpenRCT2#18970] Trying to load a non-park save will now display a context error. - Improved: [OpenRCT2#18975] Add lift sprites for steep hills on the wooden roller coaster. - Improved: [OpenRCT2#19044] Added special thanks to RMC and Wiegand to the About page. - Improved: [OpenRCT2#19131] Track missing objects when selecting scenery groups in console. - Improved: [OpenRCT2#19253] Queue junctions drawn properly when using regular paths as queue. Note: Requires using tile inspector to indicate railings can be used at T or X junctions. - Improved: [OpenRCT2#19067] New Ride window now allows filtering similarly to Object Selection. - Improved: [OpenRCT2#19272] Scenery window now allows filtering similarly to Object Selection. - Improved: [OpenRCT2#19447] The control key now enables word jumping in text input fields. - Improved: [OpenRCT2#19463] Added ‘W’ and ‘Y’ with circumflex to sprite font (for Welsh). - Improved: [OpenRCT2#19549] Enable large address awareness for 32 bit Windows builds allowing to use 4 GiB of virtual memory. - Improved: [OpenRCT2#19668] Decreased the minimum map size from 13 to 3. - Improved: [OpenRCT2#19683] The delays for ride ratings to appear has been reduced drastically. - Improved: [OpenRCT2#19697] “Show guest purchases” will now work in multiplayer. - Change: [OpenRCT2#19018] Renamed actions to fit the naming scheme. - Change: [OpenRCT2#19091] [Plugin] Add game action information to callback arguments of custom actions. - Change: [OpenRCT2#19233] Reduce lift speed minimum and maximum values for “Classic Wooden Coaster”. - Removed: [OpenRCT2#19520] Support for Windows Vista systems. - Fix: [OpenRCT2#474] Mini golf window shows more players than there actually are (original bug). - Fix: [OpenRCT2#592] Window scrollbar not able to navigate to the end of large lists. - Fix: [OpenRCT2#7210] Land tile smoothing occurs with edge tiles (original bug). - Fix: [OpenRCT2#17996] Finances window not cleared when starting some .park scenarios. - Fix: [OpenRCT2#18260] Crash opening parks that have multiple tiles referencing the same banner entry. - Fix: [OpenRCT2#18467] “Selected only” Object Selection filter is active in Track Designs Manager, and cannot be toggled. - Fix: [OpenRCT2#18904] OpenRCT2 audio object accidentally exported in saves. - Fix: [OpenRCT2#18905] Ride Construction window theme is not applied correctly. - Fix: [OpenRCT2#18911] Mini Golf station does not draw correctly from all angles. - Fix: [OpenRCT2#18971] New Game does not prompt for save before quitting. - Fix: [OpenRCT2#18986] [Plugin] Sending remote scripts larger than 63KiB crashing all clients. - Fix: [OpenRCT2#18994] Title music doesn’t start after enabling master volume. - Fix: [OpenRCT2#19025] Park loan behaves inconsistently with non-round and out-of-bounds values. - Fix: [OpenRCT2#19026] Park loan is clamped to a 32-bit integer. - Fix: [OpenRCT2#19068] Guests may not join queues correctly. - Fix: [OpenRCT2#19091] [Plugin] Remote plugins in multiplayer servers do not unload properly. - Fix: [OpenRCT2#19112] Clearing the last character in the Object Selection filter does not properly reset it. - Fix: [OpenRCT2#19112] Text boxes not updated with empty strings in Track List, Server List, and Start Server windows. - Fix: [OpenRCT2#19114] [Plugin] ‘GameActionResult’ does not comply to API specification. - Fix: [OpenRCT2#19136] SV6 saves with experimental RCT1 paths not imported correctly. - Fix: [OpenRCT2#19243] .park scenarios don’t complete properly. - Fix: [OpenRCT2#19250] MusicObjects do not free their preview images. - Fix: [OpenRCT2#19292] Overflow in ‘totalRideValue’. - Fix: [OpenRCT2#19339] Incorrect import of crashed particles from SV4. - Fix: [OpenRCT2#19379] “No platforms” station style shows platforms on the Junior Roller Coaster. - Fix: [OpenRCT2#19380] Startup crash when no sequences are installed and random sequences are enabled. - Fix: [OpenRCT2#19391] String corruption caused by an improper buffer handling in ‘GfxWrapString’. - Fix: [OpenRCT2#19434, OpenRCT2#19509] Object types added by OpenRCT2 do not get removed when executing ‘remove_unused_objects’. - Fix: [OpenRCT2#19475] Cannot increase loan when more than £1000 in debt. - Fix: [OpenRCT2#19493] SV4 saves not importing the correct vehicle colours. - Fix: [OpenRCT2#19517] Crash when peeps try to exit or enter hacked rides that have no waypoints specified. - Fix: [OpenRCT2#19524] Staff counter shows incorrect values if there are more than 32767 staff members. - Fix: [OpenRCT2#19574] Handle exits in null locations. - Fix: [OpenRCT2#19641, OpenRCT2#19643] Missing water tile in Infernal Views’ and Six Flags Holland’s river.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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 PR consists of two commits fixing issues found with Address Sanitizer:
LanguagePack::FromFileusingstd::vectorto store the loaded language file made the null terminator inexistant, but the underlying code assumed it exists. Swapping the data type forstd::stringis the easiest way to ensure the null terminator exists.GfxWrapStringcould overwrite the in/out buffer without regard for the buffer size, corrupting it. This caused a consistent assertion (or crash with ASAN) when wrapping a Japanese "Can't start construction..." string, but it is highly likely to fix many other places in the code (most notably errors) that may have been subtly broken. Unfortunately, I doubt this fixes any of the DrawScrollSummarised issues. I would love to be proven wrong on that, as it'll mean closing 50+ tickets!Closes #19330, Closes #18691, Closes #19151, Closes #18419, Closes #18062, Closes #18820, Closes #18813, Closes #18799, Closes #19384, Closes #19360, Closes #19098, Closes #19325, Closes #19313, Closes #19311, Closes #19302, Closes #19301, Closes #18557, Closes #19238, Closes #18295, Closes #19191, Closes #19173, Closes #18235, Closes #19163, Closes #19144, Closes #19138, Closes #18083, Closes #19121, Closes #18046, Closes #19064, Closes #19063, Closes #19054, Closes #19051, Closes #19029, Closes #19015, Closes #19014, Closes #19013, Closes #19012, Closes #19010, Closes #19009, Closes #19008, Closes #19005, Closes #18999, Closes #18983, Closes #18969, Closes #18950, Closes #18940, Closes #18912, Closes #18891, Closes #17439, Closes #18876, Closes #17152, Closes #17087, Closes #17267, Closes #17177, Closes #18685, Closes #18643, Closes #18633, Closes #18618, Closes #18585, Closes #18534, Closes #19052, Closes #18471, Closes #18451, Closes #18409, Closes #18291, Closes #18240, Closes #18237, Closes #19386