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

fix: gr_cls #2

Closed
wants to merge 1 commit into from
Closed

fix: gr_cls #2

wants to merge 1 commit into from

Conversation

iss000
Copy link
Contributor

@iss000 iss000 commented Mar 8, 2021

Fixes function 'gr_cls' by adding 'gr_text_w' which on first call was uninitialized.

@@ -195,7 +195,8 @@ gr_scrngeom_hires
db gr_hires_x, 0
db gr_hires_y, 0

; Only need to initialise text height, width is same as before
; Only need to initialise text height, width is same as before (iss: really? ;)
Copy link
Owner

Choose a reason for hiding this comment

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

Oops yes, you are right - the width is not initialised!

@@ -195,7 +195,8 @@ gr_scrngeom_hires
db gr_hires_x, 0
db gr_hires_y, 0

; Only need to initialise text height, width is same as before
; Only need to initialise text height, width is same as before (iss: really? ;)
db gr_text_w, 40
Copy link
Owner

@6502Nerd 6502Nerd Mar 8, 2021

Choose a reason for hiding this comment

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

However a better fix is not to initialise this here with two extra bytes, but remove the '-1' that terminates the base table and I can also remove gr_mode initialisation here as it is done in gr_scrngeom_txt.
This way the init tables are a bit smaller - which may help if other bugs are found where additional bytes are going to be needed :-)

Copy link
Owner

@6502Nerd 6502Nerd left a comment

Choose a reason for hiding this comment

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

You caught another bug - thanks! But on checking the fix, I realised the mistake is that a -1 was terminating the initialisation of the base table, if this is removed to let it continue in to the _txt part, then I can also remove some redundant bytes from the base table as they are already in the second part.
So this will be the better fix - short table = save some bytes in case needed in the future for some other fix :-)
See attachment!
2021-03-08-21-50-18

@6502Nerd 6502Nerd closed this Mar 8, 2021
@6502Nerd
Copy link
Owner

6502Nerd commented Mar 8, 2021

@iss000
Sorry I don't know how to use GitHub properly and manage pull requests etc so I hope this doesn't look rude.
But I read the suggested fix - which was indeed another bug in the graphics initialisation I didn't spot, so thank you :-)
However, it made me realise the best fix would be to adjust the table in another way which actually saves additional bytes which can come in very handy in the (near?!) future.
I really appreciate your interest, support and knowledge to look through the code, find issues and suggest fixes :-)

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.

None yet

2 participants