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

Palette does not work as expected #19

Open
ttulka opened this issue Aug 20, 2021 · 11 comments
Open

Palette does not work as expected #19

ttulka opened this issue Aug 20, 2021 · 11 comments

Comments

@ttulka
Copy link
Contributor

ttulka commented Aug 20, 2021

Consider this AS code:

import * as w4 from "./wasm4";

store<u32>(w4.PALETTE, 0xe0f8cf, 0);    // light
store<u32>(w4.PALETTE, 0xe30000, 1);    // red
store<u32>(w4.PALETTE, 0x306850, 2);    // green
store<u32>(w4.PALETTE, 0x000000, 3);    // black

export function update (): void {
  // nothing here
}

I would expect the background colored in light green, but some kind of violet appears:

image

Works fine when the other three colors are not set.

@aduros aduros added the bug Something isn't working label Aug 20, 2021
@aduros
Copy link
Owner

aduros commented Aug 20, 2021

Hmm, it turns out the 3rd param to store is in bytes and not in elements. So it should be:

store<u32>(w4.PALETTE, 0xff0000, 0);    // light
store<u32>(w4.PALETTE, 0xe30000, 4);    // red
store<u32>(w4.PALETTE, 0x306850, 8);    // green
store<u32>(w4.PALETTE, 0x000000, 12);    // black

I'll update the docs accordingly.

@aduros aduros closed this as completed in 5f98af0 Aug 20, 2021
@ttulka
Copy link
Contributor Author

ttulka commented Aug 21, 2021

Yeah, it is not very use-friendly... What about making it explicit that only four colors are supported?:

store<u32>(w4.PALETTE_1, 0xff0000);    // light
store<u32>(w4.PALETTE_2, 0xe30000);    // red
store<u32>(w4.PALETTE_3, 0x306850);    // green
store<u32>(w4.PALETTE_4, 0x000000);    // black

Some more high-level API wouldn't be quite bad, too:

w4.setPallete(0xff0000, 1);
w4.setPallete(0xe30000, 2);
w4.setPallete(0x306850, 3);
w4.setPallete(0x000000, 4);

Or even?:

w4.setPallete1(0xff0000);
w4.setPallete2(0xe30000);
w4.setPallete3(0x306850);
w4.setPallete4(0x000000);

// or...

w4.setPallete(0xff0000, 0xe30000, 0x306850, 0x000000);

@aduros
Copy link
Owner

aduros commented Aug 21, 2021

PALETTE1 to PALETTE4 sounds good!

I think we should also ask the AssemblyScript folks if there's a bug with the 3rd param to store. This documentation seems wrong https://www.assemblyscript.org/introduction.html#low-level-perspective

@ttulka
Copy link
Contributor Author

ttulka commented Aug 21, 2021

documentation seems wrong

Could you collaborate more?

The AS community is very active on Discord, that would be the best plate where to ask.

@aduros
Copy link
Owner

aduros commented Aug 21, 2021

Could you collaborate more?

In the store() example at that link, the 3rd param should be 32 instead of 8 to match the C example... I'll ping Discord about it.

@aduros aduros reopened this Aug 21, 2021
@aduros
Copy link
Owner

aduros commented Aug 21, 2021

@ttulka
Copy link
Contributor Author

ttulka commented Aug 21, 2021

Cool!

aduros added a commit that referenced this issue Aug 23, 2021
@aduros
Copy link
Owner

aduros commented Aug 23, 2021

So unfortunately the AS behavior is intended and the documentation was just wrong. I changed the WASM-4 documentation to use sizeof() to calculate the array index positions. Let's let that sit for a bit and see how it feels before adding PALETTE1-4.

@aduros aduros removed the bug Something isn't working label Aug 23, 2021
@ttulka
Copy link
Contributor Author

ttulka commented Aug 23, 2021

In Snake, I have already used

store<u32>(w4.PALETTE, 0xe0f8cf, 0 * sizeof<u32>());

But I find it plain ugly. Especially in JS-related languages such as AS, things like sizeof are very alien.

Another benefit of using PALETTE1-4 would be preventing developers from writing:

store<u32>(w4.PALETTE, 0xe0f8cf, 5 * sizeof<u32>());

It is really not a big deal, but IMHO it would increase the Developer Experience.

What speaks against that?

@eXponenta
Copy link
Contributor

eXponenta commented Sep 4, 2021

There are a lot of another ways to damage WASM memory.
You can wrap pallete inside your app on AS like this (you can remove trash code, this is generic pointer):

https://github.com/AssemblyScript/assemblyscript/blob/main/tests/compiler/std/pointer.ts

@MaxGraey
Copy link
Contributor

MaxGraey commented Sep 5, 2021

It should be

store<u32>(w4.PALETTE, 0xe0f8cf, 0 * sizeof<u32>());    // light
store<u32>(w4.PALETTE, 0xe30000, 1 * sizeof<u32>());    // red
store<u32>(w4.PALETTE, 0x306850, 2 * sizeof<u32>());    // green
store<u32>(w4.PALETTE, 0x000000, 3 * sizeof<u32>());    // black

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

No branches or pull requests

4 participants