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

Apply misc. graphics-related fixups #341

Merged
merged 1 commit into from
Sep 18, 2021
Merged

Apply misc. graphics-related fixups #341

merged 1 commit into from
Sep 18, 2021

Conversation

ISSOtm
Copy link
Member

@ISSOtm ISSOtm commented Jul 29, 2021

Up for debate, of course.

@ISSOtm ISSOtm requested a review from avivace July 29, 2021 18:50
@avivace avivace added this to In review in Development Board Jul 30, 2021
@ISSOtm
Copy link
Member Author

ISSOtm commented Sep 4, 2021

Bump

@avivace
Copy link
Sponsor Member

avivace commented Sep 13, 2021

Bump

I'm on this

src/Palettes.md Outdated Show resolved Hide resolved
src/Palettes.md Outdated Show resolved Hide resolved
src/Palettes.md Outdated Show resolved Hide resolved
src/Palettes.md Outdated Show resolved Hide resolved
src/Palettes.md Outdated Show resolved Hide resolved
src/Scrolling.md Outdated Show resolved Hide resolved
src/Scrolling.md Outdated Show resolved Hide resolved
src/Scrolling.md Outdated Show resolved Hide resolved
src/Scrolling.md Outdated
WX values 0 and 166 are unreliable due to hardware bugs.

If WX is set to 0, the window will "stutter" horizontally when SCX changes
(depending on SCX modulo 8, behavior is a little complicated so you
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Suggested change
(depending on SCX modulo 8, behavior is a little complicated so you
(depending on SCX modulo 8, detailed behaviour is unknown)

Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
(depending on SCX modulo 8, behavior is a little complicated so you
(depending on SCX modulo 8, exact behavior is a little complicated).

It is properly understood, I believe.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Then why is the reader invited to "try it himself because it's too complicated to explain"?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think because I couldn't find it documented anywhere (guess I could have taken a look at SameBoy's source?), and I thought that it wouldn't be worth explaining in full there. I was probably wrong with that, but then what shall we do?

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

We can link SameBoy source if that's implemented there.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not fond of that because AFAIK there is no test ROM for it.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

If we don't have a source, we can't provide details then we shouldn't mention it at all. Just open an Issue and mark it as research. Makes no sense to add confusion here mentioning stuff in half.

Copy link
Member Author

Choose a reason for hiding this comment

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

Then just

Suggested change
(depending on SCX modulo 8, behavior is a little complicated so you
(depending on SCX modulo 8).

?

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Agree!

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

(But still an Issue is worth opening)

src/Scrolling.md Outdated Show resolved Hide resolved
Copy link
Sponsor Member

@avivace avivace left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @ISSOtm

Co-authored-by: Antonio Vivace <dev@avivace.com>
@ISSOtm ISSOtm merged commit 5ff8ccd into gbdev:master Sep 18, 2021
@ISSOtm ISSOtm deleted the fixups branch September 18, 2021 09:07
@ISSOtm ISSOtm mentioned this pull request Sep 18, 2021
3 tasks
@ISSOtm ISSOtm moved this from In review to Done in Development Board Sep 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants