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

II-17: Initializing wCurKeys and wNewKeys #76

Merged
merged 4 commits into from
Feb 16, 2024
Merged

Conversation

kav
Copy link
Contributor

@kav kav commented Feb 1, 2024

Fixes #75

@evie-calico
Copy link
Contributor

These lds should be on the other side of the "Main" anchor

@evie-calico
Copy link
Contributor

evie-calico commented Feb 1, 2024

Oh, there isn't an "init" anchor for this chapter. I think the chapter itself needs to be corrected, not just this code.

I can take care of this for you if I have permission to edit your fork.

@kav
Copy link
Contributor Author

kav commented Feb 1, 2024

Yeah I put them there so they'd be set as part of updating main but I can add an init section instead after we define them if you'd prefer

@evie-calico
Copy link
Contributor

Yeah, that sounds best. Something like this:

We also need to initialize these when our game starts, so add two more lines:

	; Turn the LCD on
	ld a, LCDCF_ON | LCDCF_BGON | LCDCF_OBJON
	ld [rLCDC], a

	; During the first (blank) frame, initialize display registers
	ld a, %11100100
	ld [rBGP], a
	ld a, %11100100
	ld [rOBP0], a

	ld a, 0
	ld [wFrameCounter], a
	ld [wCurKeys], a
	ld [wNewKeys], a

I think diff syntax would help highlight the new lines but I'm not sure if the leading + signs would be confusing--do we use diffs anywhere else?

@kav
Copy link
Contributor Author

kav commented Feb 2, 2024

Diff syntax doesn't show up until adding the brick check two sections later.

+	call CheckAndHandleBrick

I'm inclined to instead introduce variable initialization with a sentence and a comment when initialize the frame counter and then refer to that here. (editing to note this is actually well covered so just needed to add the comment to really make sure we are buttoned)

I think diff syntax would probably be ok here but let me try the other approach first. As a new reader with fresh eyes on all this I think that would make more sense to me.

@ISSOtm
Copy link
Member

ISSOtm commented Feb 11, 2024

I can take care of this for you if I have permission to edit your fork.

Since you don't have upstream push access (something maybe we should consider? cc @avivace), an alternative is to suggest the modifications using review comments (they have an exclusive "suggest changes" feature); then it's "only" a two-click operation for the author.

Copy link
Member

@ISSOtm ISSOtm left a comment

Choose a reason for hiding this comment

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

Thank you! The change's contents look good to me; I would just like to tweak a few things before accepting it.

Though, @eievui5 mentioned that the chapter needs something rewritten? If you could look into that, then I wouldn't mind integrating the fix into this PR; but if you can't or don't want to in the next few weeks or so, then we can open a separate issue for it.

src/part2/input.md Outdated Show resolved Hide resolved
unbricked/bricks/main.asm Outdated Show resolved Hide resolved
kav and others added 3 commits February 11, 2024 13:07
Co-authored-by: Eldred Habert <eldredhabert0@gmail.com>
Co-authored-by: Eldred Habert <eldredhabert0@gmail.com>
@avivace
Copy link
Sponsor Member

avivace commented Feb 12, 2024

Thank you! The change's contents look good to me; I would just like to tweak a few things before accepting it.

Though, @eievui5 mentioned that the chapter needs something rewritten? If you could look into that, then I wouldn't mind integrating the fix into this PR; but if you can't or don't want to in the next few weeks or so, then we can open a separate issue for it.

I'd suggest we merge this and limit the scope of this PR. Evie also already has "maintain" access to this repository, although direct modifications to a feature branch/PR branch of someone else are discouraged unless:

  • the author of the PR is unresponsive / the PR is inactive since a lot of time
  • the changes are minimal/editorial

Changes may be suggested with the 'Propose changes' feature of GitHub

Copy link
Member

@ISSOtm ISSOtm left a comment

Choose a reason for hiding this comment

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

Then let's merge this improvement as it is :)

I'll let @eievui5 create an issue or PR with the changes she deems appropriate ^^

@ISSOtm ISSOtm merged commit d52e461 into gbdev:master Feb 16, 2024
2 checks passed
@ISSOtm
Copy link
Member

ISSOtm commented Feb 16, 2024

Thank you @kav!

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.

II-17 Input: Missing initialization of wCurKeys and wNewKeys
4 participants