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

Poke1 and Peek1 #1616

Merged
merged 3 commits into from
Nov 4, 2021
Merged

Poke1 and Peek1 #1616

merged 3 commits into from
Nov 4, 2021

Conversation

SuperPupperDoggo
Copy link
Contributor

I think these would be useful, especially for compact single-bit flags in save files.

@joshgoebel
Copy link
Collaborator

Though I wonder if the API should perhaps be different at that level... bitset, bitget or some such?

@StinkerB06
Copy link

This can already be done with existing peek() and poke().

@SuperPupperDoggo
Copy link
Contributor Author

SuperPupperDoggo commented Oct 6, 2021

Though I wonder if the API should perhaps be different at that level... bitset, bitget or some such?

that is a good point. poke1 and peek1 is more consistent considering that poke4 and peek4 exist, but it is also a bit weird. perhaps pokebit and peekbit would be a good name?

@SuperPupperDoggo
Copy link
Contributor Author

SuperPupperDoggo commented Oct 6, 2021

This can already be done with existing peek() and poke().

That's true, but you would have to write an function to split it into individual bits and vice versa. What you said could also be said for poke4 and peek4, and they still exist for likely the same reason as why I want to add poke1 and peek1: efficient low level control.

@joshgoebel
Copy link
Collaborator

joshgoebel commented Oct 6, 2021

The same thing could be said for poke4 and peek4, and they still exist

I think it's about balancing the need for a useful API and how common certain operation are. Since the graphics RAM is nibble based, peek4 and poke4 make a lot of sense for writing to VRAM. That argument doesn't really hold up for peek1 and poke1, which are easy enough utilities to write by hand and wouldn't add the same amount of value for the same amount of people.

So it's not the same thing.

@joshgoebel
Copy link
Collaborator

joshgoebel commented Oct 6, 2021

I mean ultimately it's @nesbox decision, but we do tend to prefer a small API. People rarely consider the COST of a larger API... assuming that adding more and more functions is "free".

@SuperPupperDoggo
Copy link
Contributor Author

SuperPupperDoggo commented Oct 6, 2021

The same thing could be said for poke4 and peek4, and they still exist

I think it's about balancing the need for a useful API and how common certain operation are. Since the graphics RAM is nibble based, peek4 and poke4 make a lot of sense for writing to VRAM. That argument doesn't really hold up for peek1 and poke1, which are easy enough utilities to write by hand and wouldn't add the same amount of value for the same amount of people.

So it's not the same thing.

I mean, you do have a point, but once again the same thing could be said for poke4 and peek4. They are very rarely used. Writing directly to nibble-based areas VRAM is not very common, because if you want to interact with individual pixels on the screen, pix is smaller in terms of size and more efficient than writing an algorithm to calculate the address of a pixel. While there is border color, most people don't even know it exists, and I have never seen it used. And a poke4 and peek4 function would also be easy enough to write by hand.

@joshgoebel
Copy link
Collaborator

While there is border color

Ah, yes, I forgot about nibble sized "registers" at various memory locations. :-)

@SuperPupperDoggo
Copy link
Contributor Author

SuperPupperDoggo commented Oct 6, 2021

While there is border color

Ah, yes, I forgot about nibble sized "registers" at various memory locations. :-)

And basically of them are either very rarely or never accessed directly because there is either an API function to do so, or the values aren't really important unless you're doing something advanced.

And did I mention most people not knowing(or forgetting) that they exist? :-)

@vsariola
Copy link
Contributor

vsariola commented Oct 7, 2021

I'll just chime in that where I come (demoscene), we use poke4 / peek4 a lot to access video memory.

But regarding the design choice of packing nibbles in memory: I would've preferred not packing two nibbles in the video memory in the first place, but do it like c64 color memory: higher nibbles never used. Similarly, registers should not pack nibbles / bits if possible. This way, only poke and peek would be needed. TIC-80 ain't really using only 64k of memory, not even close, so why bother making it more hard to do things.

Copy link
Owner

@nesbox nesbox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not to add peek2()/poke2() in this case :)
Also, you can always call peek(addr,1) instead peek1(addr), was added here #1475
Thanks

@joshgoebel
Copy link
Collaborator

I think the availability of both poke(x, 1) and peek(x,1) likely mean we should close this with "already exists"?

Should peek4 and poke4 possible be deprecated in a future release? I assume they would have never been added if this more flexible poke/peek API had already existed at that time?

@vsariola
Copy link
Contributor

vsariola commented Nov 4, 2021

Before deprecating them, let's a) profile the performance also, if the new poke api is slower to do the same thing or not. poke4/peek4 tends to be the hottest loops of them all: drawing fullscreen graphics for every pixel. b) consider if clean API is worth breaking existing cartridges, at least for a good while

@joshgoebel
Copy link
Collaborator

joshgoebel commented Nov 4, 2021

Before deprecating them, let's a) profile the performance also, if the new poke api is slower to do the same thing or not.

Ah, this is indeed a good suggestion and would perhaps be an argument for keeping. It's quite possible that poke4 and peek4 are actually SLOWER though if they are just using the underlying more complex poke API anyways. I think that's what I recall from reading the source, but I could be mistaken. Would love if someone posted a benchmark.

consider if clean API is worth breaking existing cartridges

There are many ways to deprecate things, there is no need to "break existing cartridges". Rather it could simply be that when creating new cartridges those APIs are no long accessible, etc... we already have precedent for this with things like the old palette (old cartridges get it, new ones don't), etc... nothing was broken, yet the old palette was deprecated.

It just seems very muddled to have two different API calls to do the exact same thing. At the very least we should soft deprecate one or the other - meaning we have a clear recommendation for which is "better".

@nesbox
Copy link
Owner

nesbox commented Nov 4, 2021

I am against deprecating any API, it will break compatibility with old cartridges.
I'll merge this PR and going to add poke2()/peek2() since we have peek4

@nesbox nesbox merged commit 28be7d0 into nesbox:master Nov 4, 2021
nesbox added a commit that referenced this pull request Nov 4, 2021
@nesbox
Copy link
Owner

nesbox commented Nov 4, 2021

Added peek2()/poke2() api here 2db3134

@joshgoebel
Copy link
Collaborator

joshgoebel commented Nov 5, 2021

Updating peek and poke API docs and consolidated the peek4/poke4 information there as well to avoid duplicating information everywhere. I think we can explain everything better (and in more detail) in a single place. I added redirects from the old API destinations (peek4/poke4).

The API docs do mention version 1.0 now, which is not released.... Should that we a link to a versions page where we explain which releases are out vs still in beta? I think we need to keep the API up-to-date as we add things (even if they aren't yet released)... which means there will be mentions of 1.0. I just wonder if that will lead to any confusion when someone gets looking for version 1 and can't find it?

I think we need to keep the API up-to-date as we add things

Obviously (for the API) this is helped by your "we don't deprecate thing" policy... as the only question is whether a new API or feature exists on the version someone is using, not if the behavior has changed... it's a bit nicer documenting wise.

@nesbox
Copy link
Owner

nesbox commented Nov 5, 2021

I usually add a badge with the version number in which this function works, like (0.90)

@joshgoebel
Copy link
Collaborator

Are you suggesting that instead of "added in 1.0"?

@nesbox
Copy link
Owner

nesbox commented Nov 5, 2021

Personally, I just like (1.0) 😄 but I don't mind

@joshgoebel
Copy link
Collaborator

Then I think I'll leave it for now, I like trying new things every once in a while - see how they feel. :)

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 this pull request may close these issues.

5 participants