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

WASM map() function can't be called with sx or sy = -1 #2131

Open
Madadog opened this issue Feb 17, 2023 · 3 comments · Fixed by #2132
Open

WASM map() function can't be called with sx or sy = -1 #2131

Madadog opened this issue Feb 17, 2023 · 3 comments · Fixed by #2132
Assignees

Comments

@Madadog
Copy link
Contributor

Madadog commented Feb 17, 2023

Problem

From the WASM API (tested with Rust), calling the map() function with sx or sy set to -1 replaces them with the value 0. This is not consistent with the Lua API, where -1 is a valid value. It also creates a visual discontinuity when sx or sy cross over -1.

Demonstration of visual glitches resulting from this:
video9

Solution

Remove this default-setting code from the WASM API:

TIC-80/src/api/wasm.c

Lines 688 to 689 in b4935a6

if (sx == -1) { sx = 0; }
if (sy == -1) { sy = 0; }

This does not affect any of the languages with an existing TIC80 template (C, D, Rust and Zig) because all arguments to map() must be supplied by the caller, or there will be a compilation error.

For any languages where function parameters can be omitted, this might change the default value of sx/sy to -1 (assuming m3ApiGetArg() returns -1 if an argument is missing, though I am not actually sure about this) but this can probably be worked around at the language API level when their template is added.

@joshgoebel
Copy link
Collaborator

I see two major choices here. Don't have defaults - and at the WASM binary API level you MUST specify the full params...leaving wrapper functions to make the API nicer (or not). I've already tried to adopt this approach in Zig with it's structs and pushing defaults values into the lib.

Or (since WASM is really 32bit minimally) you could change all int type params at the WASM level to i32 or u32 and have a very large sentinel value that's used as a stand-in for "DEFAULT". Of course that only works for int/bools... pointers would get more complex...

I think the first may be preferable and push this complexity into the wrapper libs... perhaps languages provider a mapAll() for "all defaults" and map(*args) for the full API, and nothing in-between... static languages often simple suck at this sort of thing more than dynamic languages. Or perhaps languages adopt a fully struct based param passing for non-required args (as the Zig wrapper does).

@nesbox
Copy link
Owner

nesbox commented Feb 20, 2023

In this case, I'd like to vote for "Don't have defaults - and at the WASM binary API level you MUST specify the full params" and define all the defaults on the wrappers level.

@Grissess
Copy link

Hey folks! I was just bitten by this bug playing with map scrolling around the origin--I figured it'd be easier to verify my math by using the origin as a convenient point. The behavior of -1 being equivalent to 0 had me suspicious of working code for the better part of an hour, and--as it has no direct workaround--is going to require me adding offsets so that the game can't practically experience this domain.

Defaults are a thorny issue, but if I may throw in my hat, I'm of the opinion that if you cannot find a reasonable "unused" value, its best not to have them at all. (A NULL pointer is "always" unused--which seems to be why pointer types proliferate even absent the need for reference. For something of this sort, though, a pointer is more burden than it's worth.)

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 a pull request may close this issue.

4 participants