Refactor formatting strings #13212

IntelOrca · 2020-10-15T23:16:06Z

Eradicate all coded format tokens. All internal strings now contain raw tokens such as {STRINGID} and {RED}. New iterators have been created to iterate the tokens and the UTF-8 codepoints.

Formatting strings has been re-written and a new template version is available.

tupaschoal · 2020-10-22T02:44:19Z

Do we want this in before the next version is released Ted?

duncanspumpkin · 2020-10-22T06:46:36Z

@tupaschoal this is bound to produce some bugs, I would say no.

IntelOrca · 2020-10-22T08:05:30Z

@tupaschoal there is no hurry for this

tupaschoal · 2020-10-22T11:02:46Z

Yep, I though so, just wanted to confirm

IntelOrca · 2020-10-22T22:04:18Z

I will apply new naming / casing suggestions.

tupaschoal

I gave up on 3def53a because I was dozing

src/openrct2/core/String.hpp

tupaschoal · 2020-11-03T02:14:53Z

src/openrct2/localisation/FormatCodes.cpp

+    auto result = std::find_if(std::begin(format_code_tokens), std::end(format_code_tokens), [token](auto& fct) {
+        return String::Equals(token, fct.token, true);
+    });
+    return result != std::end(format_code_tokens) ? result->code : 0;


Maybe use cbegin and cend?

What does cbegin and cend do?

tupaschoal · 2020-11-03T02:18:12Z

src/openrct2/localisation/FormatCodes.h

-uint32_t format_get_code(const char* token);
+#include <string_view>
+
+uint32_t format_get_code(std::string_view token);


const &? I don't think string views need them, but because you used above, I'm vouching for consistency.

@ZehMatt said to make all string_view not const &.

I know, it's just that there are other places in the diff doing const& (not sure if in the latest revision though)

src/openrct2/localisation/Formatting.cpp

tupaschoal · 2020-11-03T02:23:09Z

src/openrct2/localisation/Formatting.cpp

+        size_t num;
+        if (value < 0)
+        {
+            // TODO handle edge case: std::numeric_limits<int64_t>::min();


tupaschoal · 2020-11-03T02:36:52Z

test/tests/FormattingTests.cpp

+    auto actual = FormatString("Train is going at {VELOCITY}.", 1024);
+    ASSERT_EQ("Train is going at 1,648 km/h.", actual);
+}
+
+TEST_F(FormattingTests, velocity_mps)
+{
+    gConfigGeneral.measurement_format = MeasurementFormat::SI;
+    auto actual = FormatString("Train is going at {VELOCITY}.", 1024);


These were probably not passing (and got fixed later in the PR?)

I am not sure what you are saying. Does this test fail for you?

tupaschoal · 2020-11-03T02:43:10Z

src/openrct2/localisation/Formatting.h

+        std::string_view _str;
+        std::string _strOwned;


Don't use use TitleCase for member variables?

This is lower camel case, the convention for private fields.

tupaschoal · 2020-11-03T02:45:36Z

src/openrct2/localisation/Formatting.cpp

+        if (value < 0)
+        {
+            ss << '-';
+            value = -value;


Since we already know that it is negative, abs is unnecessary.

tupaschoal · 2020-11-03T02:50:43Z

src/openrct2/drawing/Drawing.String.cpp

+        std::string_view _str;
+        size_t _index;


This isn't title case.

tupaschoal · 2020-11-03T02:50:49Z

src/openrct2/drawing/Drawing.String.cpp

+class CodepointView
+{
+private:
+    std::string_view _str;


Don't use use TitleCase for member variables?

This isn't title case.

ZehMatt · 2020-11-05T13:48:39Z

I don't quite understand all the thread_local use, when std::string is a local variable it should be thread safe. Maybe I'm missing something about the implementation?

src/openrct2-ui/windows/TitleEditor.cpp

src/openrct2/core/String.cpp

ZehMatt · 2020-11-05T14:18:13Z

src/openrct2/core/String.cpp

@@ -154,6 +154,32 @@ namespace String
        }
    }

+    bool Equals(const std::string_view& a, const std::string_view& b, bool ignoreCase)


Suggested change

bool Equals(const std::string_view& a, const std::string_view& b, bool ignoreCase)

constexpr bool Equals(const std::string_view a, const std::string_view b, bool ignoreCase)

{

using CharType = std::string_view::value_type;

auto cmp = ignoreCase ? [](CharType a, CharType b) {

return std::tolower(a) == std::tolower(b);

} : [](CharType a, CharType b) {

return a == b;

};

return std::equal(a.begin(), a.end(), b.begin(), cmp);

}

This does not compile

src/openrct2/drawing/TTF.cpp

src/openrct2/interface/StdInOutConsole.cpp

IntelOrca · 2020-11-05T17:27:25Z

@ZehMatt Formatting calls are called from drawing calls. Drawing calls can be multi-threaded, so I am using threadlocal to ensure it doesn't share the same buffer on the heap. I am avoiding fixed static buffers because we are trying to move away from fixed numbers which may be exceeded in some scenarios or otherwise just hog stack space.

That is my reason for making it thread local over static. The reason I don't make it a local variable is to avoid memory allocation per draw frame.

Over time I would like to clean up all these functions and make them use formatter and other routines more directly which would then use a more shared thread local buffer.

ZehMatt · 2020-11-05T20:36:19Z

None of the buffers should be shared, the implementation should be thread-safe by design and not by choice. I understand the argument about allocations but the stringstream isn't helping here. The current thing with the drawing right now is that it shares a global variable as buffer which ofc requires it to be either thread_local or locking, but the goal of the Formatter was to eradicate this kind of usage. Also often when the strings are small you don't have to worry about allocations due to small string optimization.

IntelOrca · 2020-11-06T09:29:08Z

@ZehMatt how do you suggest it should be done then if we want to avoid memory allocation every single frame and avoid fixed length stack buffers?

ZehMatt · 2020-11-06T10:27:29Z

In one of my projects I do something like this

        template<typename... Args> void formatMsg(MsgType type, const char* fmt, Args&&... args)
        {
            char buffer[128];
            // Use local buffer first, for 90% of the time this is enough space.
            int actualLength = snprintf(buffer, sizeof(buffer), fmt, std::forward<Args&&>(args)...);
            if (actualLength >= sizeof(buffer))
            {
                // snprintf_s returns the size of the completely formatted string, if bigger than buffer
                // it was truncated allocate then temporary buffer.
                std::unique_ptr<char[]> tempBuf(new char[actualLength + 1]);
                _snprintf_s(tempBuf.get(), actualLength + 1, actualLength, fmt, std::forward<Args&&>(args)...);
                tempBuf[actualLength] = '\0';
                return printMsg(type, tempBuf.get());
            }
            return printMsg(type, buffer);
        }

which covers 90% of all the things going in. Also aren't the current global buffers fixed size? We can probably measure what the maximum/minimum/average is and just allocate for the worst case like above.

IntelOrca · 2020-11-06T10:30:18Z

My new global buffer is a stringstream or string which is cleared and re-used each frame, so it can grow to any size. I would like to avoid a fixed length, I would like 100% of cases to be correct, not just 90%.

If you allocate a temp buffer for this one really long string, you are still going to get that every tick that long string might be formatted.

ZehMatt · 2020-11-06T10:37:00Z

My new global buffer is a stringstream or string which is cleared and re-used each frame, so it can grow to any size. I would like to avoid a fixed length, I would like 100% of cases to be correct, not just 90%.

If you allocate a temp buffer for this one really long string, you are still going to get that every tick that long string might be formatted.

I don't know what to say, having dynamic strings comes with a cost, thread_local also comes with a cost, its probably not as expensive as an allocation but its not free either. I'm not sure why you want dynamic strings, the localisation is a fixed data set, the only exceptions for this is user supplied strings as arguments which is not often really.

Gymnasiast

OK, all my code-related questions/remarks have been solved or answered.

I still want to test this, though.

Gymnasiast

Banners from RCT1 are completely broken:

The colours are not imported correctly, there are large blank gaps and one sign even says "palelavender".

Edit: so are banners from RCT2 parks. E.g. the no entry banner on Judge Roy Scream from SF over Texas. Hovering banners also does not show the colours correctly, even on signs that otherwise work.

Gymnasiast

Most of the banner bugs are gone, but the tooltip is still not quite correct:

It should be in the colour of the banner text. In above case, it should be pale lavender.

Additionally, the banner on the left should display "Exit", but displays "Exitnce" (I made sure to check in RCT1).

Both issues are in Blackpool Pleasure Beach.

And the name of Judge Roy Scream (SF over Texas) also contains garbage.

Looks like a buffer is not cleared correctly.

IntelOrca · 2020-11-29T00:41:24Z

Issues were due to my last fix not being particularly good enough. I have now resolved those issues.

Gymnasiast

Dropdown lists lack the padding on the left, and are missing the marker that denotes the selection:

Gymnasiast

Above error appears to be the result of an error in nl-NL.txt that did not surface before (it worked by coincidence). It should be fixed in Localisation.

Saving and loading appears to work properly.

IntelOrca force-pushed the new-format-string branch 4 times, most recently from 6eeceda to 88281c4 Compare October 19, 2020 21:59

IntelOrca added the Xcode fix required label Oct 19, 2020

IntelOrca force-pushed the new-format-string branch 2 times, most recently from 55ab7d5 to 85bde11 Compare October 19, 2020 23:33

tupaschoal added this to the After v0.3.2 milestone Oct 22, 2020

IntelOrca marked this pull request as ready for review October 22, 2020 22:03

IntelOrca requested review from tupaschoal and ZehMatt October 22, 2020 22:03

IntelOrca force-pushed the new-format-string branch 3 times, most recently from 7747a44 to cac89e2 Compare November 1, 2020 22:33

tupaschoal reviewed Nov 3, 2020

View reviewed changes

tupaschoal removed this from the After v0.3.2 milestone Nov 4, 2020

IntelOrca force-pushed the new-format-string branch from cac89e2 to d275b5e Compare November 4, 2020 23:45

ZehMatt reviewed Nov 5, 2020

View reviewed changes

IntelOrca and others added 16 commits November 27, 2020 18:54

Fix more string functions

157c984

Refactor format tokens

b6a688e

Fix GCC compiler errors

92fcdfb

Fix FormatString stack memory issues

2847ac5

Add test for codepoint view

c7fea71

Fix TTF issues

167aaac

Apply review comments and fix build

fd605d7

Add Formatting.{cpp,h} to Xcode project

0dd980e

Remove includes already in header

086f10c

Apply code review comments

dfcd8a4

Handle int64 edge cases in Format

9173ec2

Improve handling of braces

ec03ce5

Do not format characters in text input

503a383

Fix title screen and object manager text issues

517e964

Improve RCT12 UTF-8 checks

9e7504f

Apply a few more code review comments

5a90cd4

IntelOrca force-pushed the new-format-string branch from 3fd8fdd to 5a90cd4 Compare November 27, 2020 18:55

Gymnasiast reviewed Nov 28, 2020

View reviewed changes

Gymnasiast requested changes Nov 28, 2020

View reviewed changes

Fix banner text formatting, import / export

699103f

Gymnasiast requested changes Nov 28, 2020

View reviewed changes

Improve banner formatting fix

17edaae

IntelOrca force-pushed the new-format-string branch from 087eaac to 17edaae Compare November 29, 2020 01:20

IntelOrca mentioned this pull request Nov 29, 2020

Feature Request: make plugin API accessible through in-game console. #13471

Open

Gymnasiast requested changes Nov 30, 2020

View reviewed changes

Gymnasiast approved these changes Dec 1, 2020

View reviewed changes

Fix OpenRCT2#13482: STR_6308 incorrect for en-GB

4187106

IntelOrca merged commit d58d834 into OpenRCT2:develop Dec 4, 2020

tupaschoal added this to the v0.3.3 milestone Dec 4, 2020

-    bool Equals(const std::string_view& a, const std::string_view& b, bool ignoreCase)
+	constexpr bool Equals(const std::string_view a, const std::string_view b, bool ignoreCase)
+	{
+		using CharType = std::string_view::value_type;
+		auto cmp = ignoreCase ? [](CharType a, CharType b) {
+			return std::tolower(a) == std::tolower(b);
+		} : [](CharType a, CharType b) {
+			return a == b;
+		};
+		return std::equal(a.begin(), a.end(), b.begin(), cmp);
+	}

Refactor formatting strings #13212

Refactor formatting strings #13212

Conversation

IntelOrca commented Oct 15, 2020

tupaschoal commented Oct 22, 2020

duncanspumpkin commented Oct 22, 2020

IntelOrca commented Oct 22, 2020

tupaschoal commented Oct 22, 2020

IntelOrca commented Oct 22, 2020

tupaschoal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZehMatt commented Nov 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IntelOrca commented Nov 5, 2020 • edited Loading

ZehMatt commented Nov 5, 2020

IntelOrca commented Nov 6, 2020

ZehMatt commented Nov 6, 2020 • edited Loading

IntelOrca commented Nov 6, 2020 • edited Loading

ZehMatt commented Nov 6, 2020

Gymnasiast left a comment

Choose a reason for hiding this comment

Gymnasiast left a comment • edited Loading

Choose a reason for hiding this comment

Gymnasiast left a comment • edited Loading

Choose a reason for hiding this comment

IntelOrca commented Nov 29, 2020

Gymnasiast left a comment

Choose a reason for hiding this comment

Gymnasiast left a comment • edited Loading

Choose a reason for hiding this comment

ZehMatt commented Nov 5, 2020 •

edited

Loading

IntelOrca commented Nov 5, 2020 •

edited

Loading

ZehMatt commented Nov 6, 2020 •

edited

Loading

IntelOrca commented Nov 6, 2020 •

edited

Loading

Gymnasiast left a comment •

edited

Loading

Gymnasiast left a comment •

edited

Loading

Gymnasiast left a comment •

edited

Loading