MEGA65 / mega65-rom-public

MEGA65 ROM public issue reporting
4 stars 0 forks source link

strange behaviour after using MOUSE command with certain parameters #118

Closed MegaNobato closed 4 months ago

MegaNobato commented 4 months ago

Test Environment

Bug Description After using the MOUSE-Command with x-position smaller 50 the mouse pointer sprite is shifted and cannot positioned to x-position smaller than 100, CTRL+V and CTRL+P does not work anymore (RUN/STOP+RESTORE does not help to activate CTRL+V and CTRL+P)

After using the Mouse command with x-positions larger 50 an error is thrown out: ?ILLEGAL QUANTITY ERROR, CTRL+V and CTRL+P works in this case

Reproduction of Issue Using direct BASIC-command: MOUSE ON,1,0,100,100 throws ?ILLEGAL QUANTITY ERROR

Using direct BASIC-command: MOUSE ON,1,0,0,0 throws no error, but mouse is placed +50 to the right: this means that the mouse is at x-position 100 but RMOUSE and RSPPOS return a result of 50 for the x-position, mouse pointer cannot moved to x-positions below 100 with mouse, additional strange behaviour: CTRL+V and CTRL+P does not work anymore

Expected behavior No ?ILLEGAL QUANTITY ERROR after correct given position on MOUSE command, no shifted mouse position and CTRL+V and CTRL+P working after using mouse command

Additional Notice ROM 920377 does not have this issue.

dansanderson commented 4 months ago

Wow, interesting. The implementation appears to have an undocumented feature in it, but this feature doesn't seem to be doing anything sensible. I'm going to make the call that the documented arguments are what's intended, and removing the undocumented feature should fix this. If we can figure out what the original intent of this code was, we can reinstate the feature with new optional arguments to the right of pos.

Another tough call: Even though this appears to be a regression from v0.95, I'm going to declare this is "will fix, but not for v0.96." We're too close to the end of the testing cycle, and this only affects MOUSE ON with a pos argument. Feedback welcome.

snoopy-f64 commented 4 months ago

There is an error in the MOUSE command in the manual.

With MOUSE ON, the optional third and fourth arguments are the coordinates of the hotspot, x=0...23 and y=0...20 (sprite size).

Only then are the details of the mouse sprite position entered.

dansanderson commented 4 months ago

Our documentation matches the original C65 design doc, and the implementation doesn't match the documentation. Because the feature doesn't work as documented, we can trust that nobody is using it, so we can do whatever we want with it. It's a judgement call.

CleanShot 2024-02-15 at 12 34 56

One option is to test and document the feature as it is currently implemented. If the feature isn't fully functional, we'll need to decide whether to finish the feature or remove it. Fealty to the original design intent is one goal of the project, but when the original implementation and documentation disagree, we have to make our own call as to what's best for the MEGA65 community.

Another option is to fix the implementation to match the documentation, then consider re-adding the undocumented feature in a way that's backwards compatible with the documented API, i.e. move the optional positional arguments to the end. This option is compelling because the latest User's Guide has already been sent to the printers. The errata is lower impact if the undocumented feature is reimplemented in a backwards compatible way with the documentation.


We just had a similar conversation about the default port value argument: the default port is 2, but nobody was relying on the default because the ports were mis-wired, and some suggested that we change the default port to 1. The original design doc is the tiebreaker, so we're keeping it as port 2.

Similarly, the VOL command is documented (by us and by the spec) to accept one argument, but the implementation actually accepts two, one for each of the C65's two SIDs. Any existing programs that use it assume that it's a master volume and may not be noticing that it isn't, so it's probably better to revise the implementation to match the documentation.

snoopy-f64 commented 4 months ago

This is the description in the - to my knowledge - most up-to-date description of the C65 "manual":

http://www.zimmers.net/anonftp/pub/cbm/manuals/c65/c65manualupdated.txt

c65manual_a

dansanderson commented 4 months ago

lol, nice. :) It's an unfortunate order of events that we documented the wrong API, but like I said if it's not working as documented then nobody is likely to be using it, so we can go either way. Allowing undocumented positional arguments to be inserted in the middle is more confusing to people using the printed manual than moving them to the end, so I think the latter option wins out. I'll check with SteerCo.

snoopy-f64 commented 4 months ago

My opinion on this: I would grant a (printed) manual exactly zero relevance to the design of the ROM or the assignment of commands. The fact that the manuals are still all "work in progress" is even included. ;-)

dansanderson commented 4 months ago

OK, I found and fixed ugly bugs in both hotspot and location features. Now that they're both working I will update the documentation.