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

MEGA65: Add nybl16 colour mode #239

Merged
merged 7 commits into from
Apr 15, 2021
Merged

MEGA65: Add nybl16 colour mode #239

merged 7 commits into from
Apr 15, 2021

Conversation

hernandp
Copy link
Contributor

Nybl16-color plus your own fixes.

@lgblgblgb
Copy link
Owner

lgblgblgb commented Apr 14, 2021

Hmm, something went wrong here :( It seems some TEXTUTRE_ macros wanted to be renamed SCREEN_ ... again :-O
Here: https://github.com/lgblgblgb/xemu/pull/239/files#r613295124
Interestingly by comparing my merger branch to your one, they seems to be identical, gives "nothing to compare".
Please see https://github.com/lgblgblgb/xemu/pull/239/files
I am not sure about the other changes it would do, but this TEXTURE vs SCREEN thinggy is very much a thing I don't understand ...

@@ -243,18 +243,20 @@ static void vic4_update_sideborder_dimensions ( void )
if (REG_CSEL) { // 40-columns?
border_x_left = FRAME_H_FRONT + SINGLE_SIDE_BORDER;
if (!REG_H640)
border_x_right = FRAME_H_FRONT + TEXTURE_WIDTH - SINGLE_SIDE_BORDER - 1;
Copy link
Owner

Choose a reason for hiding this comment

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

Here for example, your PR want to rename TEXTURE_WIDTH back to SCREEN_WIDTH, though in theory you should have for now the state where SCREEN_WIDTH (and HEIGHT) is renamed to TEXTURE...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uhmmm... i dont even remember touching that...

Copy link
Owner

@lgblgblgb lgblgblgb Apr 14, 2021

Choose a reason for hiding this comment

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

I'm a bit clueless, to be honest :( OK, maybe it was not a good idea from me to merge my changes first to you, but it still does not explain, how things changed back to SCREEN_ ... when I have no such string in the whole mega65 subtree any more ... So how I my PR changed things backwards? It does not make any sense for me :-O

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something got mixed, in my uncomitted vic4.h there were SCREEN_WIDTH and TEXTURE_WIDTH both present. Now I have the synced vic4.h with only TEXTURE_WIDTH present, changed the only 3 occurrences in vic4.c and all it's going well. Go figure what happened.

@@ -1065,6 +1063,22 @@ static void vic4_render_fullcolor_char_row ( const Uint8* char_row, int glyph_wi
}


Copy link
Owner

@lgblgblgb lgblgblgb Apr 15, 2021

Choose a reason for hiding this comment

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

Sorry for the question again, just make it clear :) So the part after this comment ( https://github.com/lgblgblgb/xemu/pull/239/files/55e202c05494a99e4b5bedca17b6145ed6dbe3a2#r614328716 ) looks like what I expected. What about the changes before? Are those really intended? I can't see it's connected to the implementation of the nybl mode video. Sorry for the so much confusion here, this turned out to be a very strange situation though the change itself for nybl mode is not so complex or anything in its own ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at the file history, that comments were suited for better debugging the NTSC/PAL mode change.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes but the !in_hypervisor again changed position what I worry for, but if you say it's intended that way, of course I believe you ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's the correct position -- may be the performance regression was due to this? e.g: recalculating modes on every frame access. The regressed output should have a lot of VIC4: border top=xxx etc.

@lgblgblgb lgblgblgb changed the title Merger MEGA65: Add nybl16 colour mode Apr 15, 2021
@lgblgblgb lgblgblgb merged commit 81bb39c into lgblgblgb:merger Apr 15, 2021
@lgblgblgb
Copy link
Owner

OK, I've merged hopefully it's fine ;) I'll check the performance now etc, what you guessed about the problem. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MEGA65: integrate Hernan's ideas/work on VIC-IV enhancements with the possibility of re-factoring meanwhile
2 participants