Skip to content

philips/mcd212.cpp: Rename matte flag constants and refactor matte handling code #13304

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

Merged
merged 4 commits into from
Feb 28, 2025

Conversation

Vincent-Halver
Copy link
Contributor

This intentionally has no visible effects. The purpose is to make the code easier to understand and maintain.

  1. The non-canonical name "Regions" was changed to the official name "Mattes". Corresponding abbreviation "R" is changed to "M".
  2. Deprecated function get_plane_width is deleted.
  3. mix_lines is refactored.
  4. Minor changes.

This intentionally makes no changes to output. The purpose is to make the code easier to understand and maintain.

1. The non-canonical name "Regions" was changed to the official name "Mattes". Corresponding abbreviation "R" is changed to "M".
2. Deprecated function get_plane_width is deleted.
3. mix_lines is refactored.
4. Minor changes.
This pull request intentionally has no behavior changes.

To ensure this pull has no behavior changes, we pause the refactoring here so that these non-visible changes can be accepted first.

Reduction of code (-10 lines).
@Vincent-Halver
Copy link
Contributor Author

Any concerns?

Fixes style. update_matte_arrays will need further refactoring later.
The declaration of x has to be outside the loop because it's used below the loop.
@Slamy
Copy link
Contributor

Slamy commented Feb 27, 2025

@Vincent-Halver Are you sure you want to rename regions to mattes? At least according to the datasheet of the MCD212 the name region is actually used in there. I'm asking as this might confuse future readers of mcd212.cpp

@Vincent-Halver
Copy link
Contributor Author

Vincent-Halver commented Feb 27, 2025

@Slamy I feel reasonably sure, though I see why you ask. Let me explain. Mattes and Regions are closely related in the spec:

From the Greenbook Spec:

2.3.2.6 Regions
Regions divide the set of points in the UCM coordinate space into two disjoint sets
of points. Their main uses are for graphics drawing and construction of mattes.

2.3.4.3 Regions
Regions divide a drawmap into two disjoint sets of pixels. Their primary use is to
limit the area of drawing in a drawmap. Regions can also be drawn. Regions may
be of any arbitrary shape or size. Regions are created from the basic shapes of
rectangle, polygon, circle, circular wedge, ellipse, and elliptical wedge. More
complex shapes are created from these basic shapes using intersection, union,
exclusive-or, and difference operations. (see VII.2.3.2.6 for more information).

The various function names in the OS call these shapes and unions/intersections/etc as "regions".

RG_Creat - Create Region
RG_Creat creates a region using one of the basic shapes.
Input: d0.w = Path number
d1.w = SS_RG setstat code
d2.w = RG_Creat function code
d3.w = Region type (rectangle, polygon, etc.)
d4.l = Region type dependent
d5.l = Region type dependent
d6.l = Region type dependent
d7.l = Region type dependent
(a0) = Region type dependent

The Validation Disc also uses this terminology. You may see debug output when using that disc that refers to these function names. Regions are small files that occupy space in the file manager and developers have to allocate and deallocate them.

I do see that the MCD212 PDF doesn't use the word Matte, and instead calls them regions. That means that a choice would have to be made: do you use the Motorola chip's terms, or the official Philips terms?

I think it would be good to consider who would actually be reading and writing the code. When working on the emulator, the Validation Disc, Operating System, and official CD-i specification all use the term Matte. It reduces developer confusion to use the same terms that the software uses. So I come to the conclusion it should follow the Greenbook terminology.

@Slamy
Copy link
Contributor

Slamy commented Feb 27, 2025

@Vincent-Halver As I'm not the maintainer of this code, I also don't have a saying in this. I was just wondering, considering MAME is not using the HLE approach and sees the systems more low level. In theory CD-i is a software standard and there I would totally understand the naming being done like in the green book.
The same problem exists in the src/mame/philips/cdicdic.cpp. The green book says it is a soundmap while the mame code calls it the audiomap. As there is no public datasheet, nobody really knows... :-(
Are you a member of the CD-i discord server? I'm actually observing your actions, considering the state of CD-i emulation is not the best right now. I've also talked to MooglyGuy some time ago about the MAME CD-i code. He seems to have lost interest though...

@angelosa
Copy link
Member

Green Book is a standard, and it's not confined to Philips CD-i alone.

The same problem exists in the src/mame/philips/cdicdic.cpp. The green book says it is a soundmap while the mame code calls it the audiomap.

Those are interchangeable names tbh, there isn't a real convention there. It's all about personal preferences, I prefer to use actual doc naming for stuff, except where that turns out to be impractical or confusing for MAME.

@Vincent-Halver
Copy link
Contributor Author

Are you a member of the CD-i discord server

I am not, but I would join. Something that would help me a lot would be someone to record all the visual tests from Validation Disc (E) on real hardware.

It sounds like we're ok with using the Greenbook spec naming, so if that's alright, let's pull in the change, and I'll clean up the next section of code that fixes the Matte execution order that I alluded to above.

@cuavas cuavas changed the title CD-i: Refactoring and Renaming philips/mcd212.cpp: Rename matte flag constants and refactor matte handling code Feb 28, 2025
@cuavas cuavas merged commit f074104 into mamedev:master Feb 28, 2025
5 checks passed
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.

4 participants