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

Add PCjr CGA composite output (1 of 2) #1122

Merged
merged 9 commits into from Jul 8, 2021
Merged

Conversation

kcgen
Copy link
Member

@kcgen kcgen commented Jul 5, 2021

Merges NewRisingSun's PCjr composite patch. This addresses the first part of the eXoDOS composite project card.

Suggest reviewing commit-by-commit.

  • The first commit is the patch verbatim.
  • Follow on commits only make small changes (formatting, comments, types.. no renaming or logic changes)

A follow on PR addresses compiler warnings in this file, in general (to keep this PR scoped to just this feature).

2021-07-05_15-02_1
2021-07-05_15-02
2021-07-05_15-00

@kcgen kcgen added the enhancement New feature or enhancement of existing features label Jul 5, 2021
@kcgen kcgen requested review from dreamer and kklobe July 5, 2021 22:18
@kcgen kcgen self-assigned this Jul 5, 2021
@kcgen kcgen added this to In progress in 0.78 release via automation Jul 5, 2021
@kcgen kcgen changed the title Add PCjr CGA composite output Add PCjr CGA composite output [1 of 2] Jul 5, 2021
@kcgen kcgen force-pushed the kc/nrs-pcjr-composite-1 branch 2 times, most recently from 0b219fe to ae32b80 Compare July 6, 2021 03:52
@kcgen
Copy link
Member Author

kcgen commented Jul 6, 2021

Originally NRS and the other composite folks had included conf settings to dial in composite mode, but there were concerns that this would impact the patches likelihood of being merged - so they fell back to hotkey controls.
I enjoy tinkering with them, but from a preservation stans-point, I can see the value the conf settings provide. A project like eXo could offer composite in the startup selection list, and the game would come up with all settings adjusted to most faithfully reproduce it.
With hotkeys, eXo will have to say "remember to press F12, followed by Ctrl+F11 seventeen times until the colors are blue, orange, and dark green.(or something like that... There are hundreds of hue step).
I unfortunately don't have a good suggestions for the conf settings though, if we wanted to add them.

Copy link
Member

@dreamer dreamer left a comment

Choose a reason for hiding this comment

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

The commit message in first commit should include line:

Imported-from: https://www.vogons.org/viewtopic.php?p=546614#p546614

(not Ref: line).

Left few tiny comments - overall the code looks good and you cleaned it up nicely :) I haven't tested it yet, but we need to make sure there's no accidental regression when turning on emulator in PCjr or Tandy modes.

src/hardware/vga_other.cpp Outdated Show resolved Hide resolved
src/hardware/vga_other.cpp Outdated Show resolved Hide resolved
src/hardware/vga_other.cpp Outdated Show resolved Hide resolved
src/hardware/vga_other.cpp Show resolved Hide resolved
@dreamer
Copy link
Member

dreamer commented Jul 6, 2021

Originally NRS and the other composite folks had included conf settings to dial in composite mode, but there were concerns that this would impact the patches likelihood of being merged - so they fell back to hotkey controls.
I enjoy tinkering with them, but from a preservation stans-point, I can see the value the conf settings provide. A project like eXo could offer composite in the startup selection list, and the game would come up with all settings adjusted to most faithfully reproduce it.
With hotkeys, eXo will have to say "remember to press F12, followed by Ctrl+F11 seventeen times until the colors are blue, orange, and dark green.(or something like that... There are hundreds of hue step).
I unfortunately don't have a good suggestions for the conf settings though, if we wanted to add them.

Yes, we should add .conf controls for CGA modes, but we need to do it carefully, and make sure they can be switched via config command. Pedantically speaking they would belong to machine section, but from usability standpoint perhaps render section would work better (as we have monochrome_palette option in there already). Not in scope for this PR though. Before finishing implementation of new .conf we should talk to eXo and treat him as a client for this feature.

@kcgen
Copy link
Member Author

kcgen commented Jul 6, 2021

Yes, we should add .conf controls for CGA modes, but we need to do it carefully, and make sure they can be switched via config command. Pedantically speaking they would belong to machine section, but from usability standpoint perhaps render section would work better (as we have monochrome_palette option in there already). Not in scope for this PR though. Before finishing implementation of new .conf we should talk to eXo and treat him as a client for this feature.

Really like this approach - thanks @dreamer.

Copy link
Collaborator

@kklobe kklobe left a comment

Choose a reason for hiding this comment

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

It looks like a lot of variables in update_cga16_color can be const or constexpr, any benefit to doing that now?

e.g.:

	constexpr double tau = 6.28318531; // == 2*pi
	constexpr double ns = 567.0/440;  // degrees of hue shift per nanosecond

@kcgen
Copy link
Member Author

kcgen commented Jul 6, 2021

It looks like a lot of variables in update_cga16_color can be const or constexpr, any benefit to doing that now?

e.g.:

	constexpr double tau = 6.28318531; // == 2*pi
	constexpr double ns = 567.0/440;  // degrees of hue shift per nanosecond

Thanks! Yes; I think they're perfect for the part-2 PR that passes over the entire file.

@kcgen
Copy link
Member Author

kcgen commented Jul 6, 2021

Updates inbound.

@kcgen kcgen changed the title Add PCjr CGA composite output [1 of 2] Add PCjr CGA composite output (1 of 2) Jul 8, 2021
@kcgen
Copy link
Member Author

kcgen commented Jul 8, 2021

CGA composite continues without issues. Here are some machine = cga screenshot, which match the main branch.

2021-07-07_16-17

2021-07-07_16-17_1

@dreamer
Copy link
Member

dreamer commented Jul 8, 2021

It looks like a lot of variables in update_cga16_color can be const or constexpr, any benefit to doing that now?

e.g.:

	constexpr double tau = 6.28318531; // == 2*pi
	constexpr double ns = 567.0/440;  // degrees of hue shift per nanosecond

Good catch - I think we can shift it to the next PR :) Whenever we can use costexpr or const - we should.

Whenever dealing with "standard" know math constants like Pi, it's preferable to use constants from cmath, so:

	constexpr double tau = 2 * M_PI;

@dreamer
Copy link
Member

dreamer commented Jul 8, 2021

@kcgen Looks great - thanks! :)

@dreamer dreamer merged commit 0c0c819 into master Jul 8, 2021
0.78 release automation moved this from In progress to Done Jul 8, 2021
@Wengier
Copy link
Sponsor Collaborator

Wengier commented Jul 8, 2021

Glad to see this feature got added.

@kklobe
Copy link
Collaborator

kklobe commented Jul 8, 2021

Whenever dealing with "standard" know math constants like Pi, it's preferable to use constants from cmath, so:

	constexpr double tau = 2 * M_PI;

This is historically an issue for Visual Studio, speaking from past experience, so might be why the original author avoided it.

@dreamer
Copy link
Member

dreamer commented Jul 8, 2021

Whenever dealing with "standard" know math constants like Pi, it's preferable to use constants from cmath, so:

	constexpr double tau = 2 * M_PI;

This is historically an issue for Visual Studio, speaking from past experience, so might be why the original author avoided it.

Not for us, we fixed it for VS already :)

It's not really a problem in VS but rather with projects using incorrect include order in header and cpp files - it just manifests in VS more often than anywhere else.

@kcgen kcgen deleted the kc/nrs-pcjr-composite-1 branch July 9, 2021 02:18
@kcgen kcgen added this to the 0.78 release milestone Dec 21, 2021
@GranMinigun GranMinigun added this to Done in eXoDOS features Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or enhancement of existing features
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants