search

MEGA65 / mega65-core

MEGA65 FPGA core
Other
240 stars 85 forks source link

rrb gotox is off by 1 #337 #627

Closed ki-bo closed 1 year ago

ki-bo commented 1 year ago

This PR fixes X positions in RRB GOTOX being one pixel off to the right. The fix may be important once programs rely on pixel exact positioning of RRB layers, and developers should not be encouraged to work around that bug by intentionally positioning their layers one pixel off.

lydon42 commented 1 year ago

Are you positive that there are no side effects from this (except breaking programs that work around the bug)?

ki-bo commented 1 year ago

I know this is a very trivial way of fixing this issue, so I encourage everyone to do a review whether another fix would target this better.

Regarding side-effects, I see it that way:

I must admit, I don't have an analytical explanation why the former implementation was providing layers being one pixel off. But my rationale is: if we always are 1 pixel off, the most safe way around the bug is correcting the user provided gotox value by one. It is the same as if the user had provided the manually corrected value.

I specifically tested around edge cases and wrap-arounds. That is, we have 10 bits of gotox position, so I tested all kinds of gotox around 0 (3ff and below, as well as 0 and above), which provided the correct results in my experiments.

gardners commented 1 year ago

Hello, while your fix would certainly fix the technical problem, after discussion in the steering group, the decision has been made that it would be too disruptive to fix this issue now. Shallan, Retrocogs, Mirage, myself and others have all made software that assumes the current behaviour, thus there would be some level of inconvenience caused by fixing it. In contrast, the fault only changes the value you have to put in the GOTOX token, rather than preventing any functionality. Thus the risk/effort vs reward ratio is unfortunately not compelling. Also, it wouldn't be the only such "out by one" issue in the C65/M65 video system: The C65's VIC-III already has a hardware out-by-one error with the soft scrolling position in H640 mode being out by one pixel. Thus, while we very much appreciate the analysis, and pull request with proposed fix, we won't be merging it in. But please don't let this discourage you from proposing other improvements and fixes -- it is really valuable! Thanks, Paul.