Skip to content
This repository was archived by the owner on Apr 29, 2024. It is now read-only.

Conversation

@Hammie
Copy link
Member

@Hammie Hammie commented Jul 21, 2022

Adds a LegacyDialog entity that implements the old Seadogs 2 dialog system for use in New Horizons.

Does not yet include pagination or mood settings for the head animation.

Also includes #383

@q4a
Copy link
Contributor

q4a commented Jul 22, 2022

@Hammie sorry, but clang is more strict. If you create td::vector<LinkEntry> formattedLinks_; then you need to emplace_back only LinkEntry. I fixed problem, builded on Linux and tested a bit.
Works fine, thanks!
2ff2085

             for (const auto &text : link_texts)
             {
-                formattedLinks_.emplace_back(text, static_cast<int32_t>(i));
+                formattedLinks_.emplace_back(LinkEntry{text, static_cast<int32_t>(i)});
             }

@Hammie
Copy link
Member Author

Hammie commented Jul 22, 2022

@q4a Thank you very much!

Nothing quite like waking up to see problems already solved 👍

Copy link
Member

@espkk espkk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good!
I see you removed some utf8-related code from the original Dialog though. Did you mean to drop its support or it's still supported somehow?

PS
@q4a, iirc from our discussion clang is not more strict, it just doesn't support this C++20 feature yet 😅

@Hammie
Copy link
Member Author

Hammie commented Jul 24, 2022

I see you removed some utf8-related code from the original Dialog though. Did you mean to drop its support or it's still supported somehow?

@espkk I assume you are talking about the modified AddToStringArrayLimitedByWidth? I'm confident the new code still works with utf-8, but I will try and find a more conclusive way to make sure.

@Hammie Hammie marked this pull request as draft August 7, 2022 11:43
@q4a q4a closed this Aug 7, 2022
@q4a q4a reopened this Aug 7, 2022
@Hammie
Copy link
Member Author

Hammie commented Aug 28, 2022

@espkk I've tried creating a unit test to make sure it still works with non-ascii characters, but the presence of the renderer makes that almost impossible.

The theory as to why it should still work is that neither ' ' or '\n' could ever by confused for other characters in utf-8 - as is the case for all ascii characters <= code point 127 - because multi-byte code points always have the highest bit active.

Also did a quick test with some Cyrillic text, no idea what it says but it looks like it is rendering correctly:
Sea Dogs 28_08_2022 14_28_55

@Hammie Hammie marked this pull request as ready for review August 28, 2022 12:57
@espkk
Copy link
Member

espkk commented Aug 31, 2022

@espkk I've tried creating a unit test to make sure it still works with non-ascii characters, but the presence of the renderer makes that almost impossible.

The theory as to why it should still work is that neither ' ' or '\n' could ever by confused for other characters in utf-8 - as is the case for all ascii characters <= code point 127 - because multi-byte code points always have the highest bit active.

Also did a quick test with some Cyrillic text, no idea what it says but it looks like it is rendering correctly

Sounds good! Thanks

But this needs a rebase now 😢

espkk
espkk previously approved these changes Aug 31, 2022
Copy link
Member

@espkk espkk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only 1 minor note - please move new header ito src if it's not public
include is for public headers which are getting exported

# Conflicts:
#	src/libs/dialog/src/dialog.cpp
#	src/libs/renderer/include/dx9render.h
@espkk espkk merged commit 726d3bb into develop Sep 17, 2022
@espkk espkk deleted the feature/legacy-dialog branch September 17, 2022 07:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants