Skip to content

Fix #13611: Zero under decimal is not displayed#13618

Merged
duncanspumpkin merged 5 commits into
OpenRCT2:developfrom
csunday95:develop-bug-13611-fixed-point-display
Dec 22, 2020
Merged

Fix #13611: Zero under decimal is not displayed#13618
duncanspumpkin merged 5 commits into
OpenRCT2:developfrom
csunday95:develop-bug-13611-fixed-point-display

Conversation

@csunday95

Copy link
Copy Markdown
Contributor

The existing code does not properly handle the formatting conversion case for fixed point decimal values. If a fixed point integer representation has fewer significant figures than the required fixed point precision, no zeros are placed between the decimal place and the first sig fig. This causes ride intensity, nausea etc. to display incorrectly when very small values, e.g. a nausea value of 0.05 will be displayed as 0.5.

@IntelOrca

Copy link
Copy Markdown
Contributor

Are you able to write a unit test for this?

@csunday95

Copy link
Copy Markdown
Contributor Author

yes, but I was trying to figure out where the format for it is specified. I assume I can just add a TEST_F method to the tests/FormattingTests.cpp file?

// handle case where value has fewer sig figs than required decimal places
while (num == 0 && i < TDecimalPlace && i < sizeof(buffer))
{
buffer[i++] = static_cast<char>('0');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Surely the static cast is not required.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes certainly not required, just put it there for consistency/readability with the prior line that makes use of '0' char literal as an offset: static_cast<char>('0' + (num % 10)). Removing the static cast probably won't have any effect on code generation, but if you'd prefer it be removed for style I can.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree, just remove it if it isn't required.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have removed the explicit cast; I made my commit summary message slightly too long though and got caught by the CI linter; is there anything I need to do about that (e.g. commit --amend) or is it more of a reminder?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just amend the commit the Mac os ci has failed as well. Might be unrelated though.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Mac CI mysteriously had a connection timeout during upload phase from what I could tell, might just be a random glitch with CI

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah Mac CI always seems to have issues.

@Gymnasiast Gymnasiast changed the title fix for bug #13611; handled too few sig figs fixed point case Fix #13611: Zero under decimal is not displayed Dec 21, 2020
 - already a char literal and assigning to char[] so code
 is functionally equivalent
@csunday95 csunday95 force-pushed the develop-bug-13611-fixed-point-display branch from c06514a to f3b21fe Compare December 21, 2020 19:47
@duncanspumpkin duncanspumpkin merged commit fa5437f into OpenRCT2:develop Dec 22, 2020
@tupaschoal tupaschoal added this to the v0.3.3 milestone Dec 22, 2020
@csunday95 csunday95 deleted the develop-bug-13611-fixed-point-display branch December 22, 2020 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants