tracy icon indicating copy to clipboard operation
tracy copied to clipboard

Consistently print fixed-width percentages.

Open bjacob opened this issue 2 years ago • 6 comments

I have run into the assertion being removed here. Apparently it was assuming that a float value was printing as a specific number of characters and that assumption was defeated by some float value.

Specifically, this triggerred with rate=0.099671, just below the 0.1f threshold it's compared against here. We print rate*100 = 9.9671f as the 4 characters 10.0.

bjacob avatar May 04 '22 16:05 bjacob

This is probably why you had layout issues. Why is the assertion no longer needed?

wolfpld avatar May 04 '22 19:05 wolfpld

I was running into the layout issue much more often than into this assertion --- definitely many cases had the layout issue even though the assertion was successful.

I'm not sure about whether the assertion is needed because I'm not 100% sure what it was for. IIUC it was there to ensure correct right-justification of the printed numbers (so the one space prepended in one of the if branches is indeed the right amount of whitespace to prepend)?

What I am saying though is that the assertion was unsafe -- when the rate value is just below 0.1f, as it was here rate=0.099671, we take the if branch for values smaller than 0.1f expecting only 3 characters to be printed but rounding may still result in the 4 characters 10.0 being printed.

bjacob avatar May 04 '22 19:05 bjacob

IIUC it was there to ensure correct right-justification of the printed numbers

Yes.

What I am saying though is that the assertion was unsafe

I understand, I have seen this manifest before.

My worry is that the change only removes the assert without any logic changes. I would rather see this being bulletproofed to always produce the expected result. It is how it is because the proper solution is not trivial to work out and the issue was rare to see on my end.

wolfpld avatar May 04 '22 20:05 wolfpld

Ah ok. I had actually been hesitating between a minimal drop-in fix (this PR now) and something more generally correct so I'll do the latter now and update this PR.

bjacob avatar May 04 '22 20:05 bjacob

@wolfpld FYI - this is ready for another round of reviewing.

bjacob avatar May 12 '22 17:05 bjacob

The changes miss some of important logic that was present before. For example, the values are no longer clamped to no more than 100%.

wolfpld avatar May 14 '22 10:05 wolfpld

This should be fixed by 86c25748.

wolfpld avatar Aug 14 '22 12:08 wolfpld