Skip to content
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

Add names to the list of authors #142

Merged
merged 5 commits into from
Jan 20, 2025

Conversation

realSquidCoder
Copy link
Collaborator

@ab9rf Please add your name too if you wish it to be included, you deserve this as much as I, if not more so. :)

@realSquidCoder realSquidCoder changed the title Add my name to the list Add names to the list of authors Jan 16, 2025
@realSquidCoder realSquidCoder marked this pull request as ready for review January 16, 2025 17:02
@@ -1 +1 @@
7� Nickel, BatCountry, Belal, Belannaer, Caldfir, DeKaFu, Dante, Deon, dyze, Errol, fifth angel, frumpton, IDreamOfGiniCoeff, Impaler, Japa, jarathor, Jiri Petru, Jordix, Lord Nightmare, McMe, Mike Mayday, Nexii Malthus, peterix, Seuss, soup, Talvara, winner, Xandrin,
7� Nickel, BatCountry, Belal, Belannaer, Caldfir, DeKaFu, Dante, Deon, dyze, Errol, fifth angel, frumpton, IDreamOfGiniCoeff, Impaler, Japa, jarathor, Jiri Petru, Jordix, Lord Nightmare, McMe, Mike Mayday, Nexii Malthus, peterix, Seuss, soup, SquidCoder, Talvara, winner, Xandrin,
Copy link
Member

Choose a reason for hiding this comment

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

do you know what that character is supposed to be for 7c Nickel? It is displayed in the UI as a c, both before and after this change, but in reality, this PR changes the character in the file:
image

that is, it's being changed from cp437 character 0xA2 to what looks like a unicode character. I'd look into the expected encoding of this file to make sure this is a correct change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i think its a "cent" sign?

Copy link
Member

Choose a reason for hiding this comment

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

what encoding does it need to be in for display?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

idk but i can tell you its a cent sign by looking at the df wiki:
image

Copy link
Member

@myk002 myk002 Jan 19, 2025

Choose a reason for hiding this comment

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

My question is: what encoding does the allegro function that renders the text expect? Let's use that encoding.

Copy link
Member

Choose a reason for hiding this comment

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

oh, n/m this file isn't read, is it? This should be UTF-8, then, and my question applies to line 199 in main.cpp

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i think, there, its literally just the letter c

Copy link
Member

Choose a reason for hiding this comment

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

yeah, but that's incorrect, isn't it? it should be the cent sign

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

its been the c for as long as i know

main.cpp Outdated Show resolved Hide resolved
main.cpp Show resolved Hide resolved
Contributions.txt Outdated Show resolved Hide resolved
@@ -199,9 +199,9 @@ void drawcredits()
al_draw_text(font, color_white, centerx, bottomy-12*lineheight, ALLEGRO_ALIGN_CENTRE, "7c Nickel, BatCountry, Belal, Belannaer, DeKaFu, Dante, Deon, dyze,");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
al_draw_text(font, color_white, centerx, bottomy-12*lineheight, ALLEGRO_ALIGN_CENTRE, "7c Nickel, BatCountry, Belal, Belannaer, DeKaFu, Dante, Deon, dyze,");
al_draw_text(font, color_white, centerx, bottomy-12*lineheight, ALLEGRO_ALIGN_CENTRE, " Nickel, BatCountry, Belal, Belannaer, DeKaFu, Dante, Deon, dyze,");

Copy link
Member

Choose a reason for hiding this comment

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

From the allegro docs:

Some parts of the Allegro API, such as the font rountines[sic], expect Unicode strings encoded in UTF-8.

Copy link
Member

@ab9rf ab9rf Jan 20, 2025

Choose a reason for hiding this comment

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

MSVC will (most likely) encode that string in CP-1252, not UTF-8; ¢ has an encoding in CP-1252, but that encoding will yield a byte sequence that won't decode to ¢ as UTF-8.

You need to use a u8 encoding override if you want to guarantee the compiler encodes that as a UTF-8 string.

I also can't guarantee that al_draw_text will properly handle UTF-8. There is an al_draw_ustr which takes an ALLEGRO_USTR object as its parameter. ALLEGRO_USTR is allegro's type for representing UTF-8 strings.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i'd rather leave it as "c" if we're unsure that it'll work

@myk002 myk002 merged commit 61a47f9 into DFHack:master Jan 20, 2025
4 checks passed
@realSquidCoder realSquidCoder deleted the update-stonesense-authors branch January 25, 2025 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants