[WIP] feature: VESA VBE 1.2 by maximilien-noal · Pull Request #1587 · OpenRakis/Spice86

maximilien-noal · 2025-12-03T21:45:04Z

Description of Changes

Introduces VBE VESA 1.2 extensions to the VGA BIOS.

Rationale behind Changes

Some games try to use it (like Rules of Engagement 2)

Suggested Testing Steps

Already tested with Rules of Engagement 2.

Copilot

Pull request overview

This PR implements VESA BIOS Extension (VBE) 1.2 support for the VGA BIOS emulator, enabling compatibility with games like Rules of Engagement 2 that require Super VGA functionality. The implementation adds standardized VESA functions for querying and setting video modes beyond standard VGA capabilities.

Key Changes:

Implements VBE 1.2 specification with three core functions: GetControllerInfo (00h), GetModeInfo (01h), and SetMode (02h)
Adds memory-mapped data structures (VbeInfoBlock, VbeModeInfoBlock) for VESA information exchange
Refactors existing VGA BIOS code to replace magic numbers with named constants for improved maintainability

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.

File	Description
`VgaBios.cs`	Adds VBE 1.2 implementation with three new functions, two nested data structure classes, and extensive constant definitions. Refactors existing code to use named constants instead of magic numbers.
`IVesaBiosExtension.cs`	Defines the interface contract for VESA BIOS Extension functions with comprehensive documentation from the VBE 1.2 specification.

Copilot · 2025-12-03T21:49:00Z

src/Spice86.Core/Emulator/InterruptHandlers/VGA/VgaBios.cs

+        }
+
+        // Return success
+        State.AX = 0x004F;


Magic number 0x004F should be replaced with (ushort)VbeStatus.Success to match the pattern used elsewhere in the VBE implementation (lines 1448, 1587, etc.). This improves consistency and code maintainability.

Copilot · 2025-12-03T21:49:00Z

src/Spice86.Core/Emulator/InterruptHandlers/VGA/VgaBios.cs

+                modeInfo.RedFieldPosition = (byte)(bpp == 16 ? 11 : 10);
+                modeInfo.GreenMaskSize = (byte)(bpp == 16 ? VbeModeInfoConstants.GreenMaskSize6Bit : VbeModeInfoConstants.RedGreenBlueMaskSize);
+                modeInfo.GreenFieldPosition = 5;
+                modeInfo.BlueMaskSize = VbeModeInfoConstants.RedGreenBlueMaskSize;
+                modeInfo.BlueFieldPosition = 0;
+            } else if (bpp == 24 || bpp == 32) {
+                modeInfo.RedMaskSize = VbeModeInfoConstants.RedGreenBlueMaskSize8;
+                modeInfo.RedFieldPosition = 16;
+                modeInfo.GreenMaskSize = VbeModeInfoConstants.RedGreenBlueMaskSize8;
+                modeInfo.GreenFieldPosition = 8;
+                modeInfo.BlueMaskSize = VbeModeInfoConstants.RedGreenBlueMaskSize8;
+                modeInfo.BlueFieldPosition = 0;


Magic numbers for bit field positions should be extracted to named constants in VbeModeInfoConstants. Define constants for:

Red field positions: 10 (15-bit), 11 (16-bit), 16 (24/32-bit)

Green field positions: 5 (15/16-bit), 8 (24/32-bit)

Blue field position: 0

For example:

public const byte RedFieldPosition15Bit = 10; public const byte RedFieldPosition16Bit = 11; public const byte RedFieldPosition24Bit = 16;

This makes the bit layout explicit and easier to understand.

Copilot · 2025-12-03T21:49:01Z

src/Spice86.Core/Emulator/InterruptHandlers/VGA/VgaBios.cs

+        modeInfo.YResolution = height;
+        modeInfo.XCharSize = VbeModeInfoConstants.CharWidth;
+        modeInfo.YCharSize = VbeModeInfoConstants.CharHeight;
+        modeInfo.NumberOfPlanes = (byte)(bpp == 4 ? 4 : 1);


Magic number 4 for planar mode plane count should be extracted to a named constant in VbeModeInfoConstants, e.g., PlanarModePlanes = 4. This makes the planar mode plane count explicit and self-documenting.

src/Spice86.Core/Emulator/InterruptHandlers/VGA/VgaBios.cs

Copilot · 2025-12-03T21:49:01Z

src/Spice86.Core/Emulator/InterruptHandlers/VGA/VgaBios.cs

+        }
+
+        /// <summary>
+        /// "The XCharCellSize and YCharSellSize specify the size of the character cell in


Spelling error in quoted specification text: "YCharSellSize" should be "YCharCellSize" (typo appears twice in lines 604 and 614).

Copilot · 2025-12-03T21:49:01Z

src/Spice86.Core/Emulator/InterruptHandlers/VGA/VgaBios.cs

+        }
+
+        /// <summary>
+        /// "The XCharCellSize and YCharSellSize specify the size of the character cell in


Spelling error in quoted specification text: "YCharSellSize" should be "YCharCellSize".

Copilot · 2025-12-03T21:49:02Z

src/Spice86.Core/Emulator/InterruptHandlers/VGA/VgaBios.cs

+        /// </summary>
+        public const byte CharWidth = 8;
+        /// <summary>
+        /// "The YCharSellSize...size of the character cell in pixels" = 16


Spelling error in quoted specification text: "YCharSellSize" should be "YCharCellSize".

Suggested change

/// "The YCharSellSize...size of the character cell in pixels" = 16

/// "The YCharCellSize...size of the character cell in pixels" = 16

Copilot · 2025-12-03T21:49:02Z

src/Spice86.Core/Emulator/InterruptHandlers/VGA/VgaBios.cs

+        return mode switch {
+            // VBE 1.2 standard modes - return info for queries
+            // Note: Only mode 0x102 can actually be SET (has internal VGA mode support)
+            0x100 => (640, 400, 8, true),    // 640x400x256
+            0x101 => (640, 480, 8, true),    // 640x480x256
+            0x102 => (800, 600, 4, true),    // 800x600x16 (planar) - CAN BE SET via mode 0x6A
+            0x103 => (800, 600, 8, true),    // 800x600x256
+            0x104 => (1024, 768, 4, true),   // 1024x768x16 (planar)
+            0x105 => (1024, 768, 8, true),   // 1024x768x256
+            0x106 => (1280, 1024, 4, true),  // 1280x1024x16 (planar)
+            0x107 => (1280, 1024, 8, true),  // 1280x1024x256
+            0x10D => (320, 200, 15, true),   // 320x200x15-bit
+            0x10E => (320, 200, 16, true),   // 320x200x16-bit
+            0x10F => (320, 200, 24, true),   // 320x200x24-bit
+            0x110 => (640, 480, 15, true),   // 640x480x15-bit (S3 mode 0x70)
+            0x111 => (640, 480, 16, true),   // 640x480x16-bit
+            0x112 => (640, 480, 24, true),   // 640x480x24-bit
+            0x113 => (800, 600, 15, true),   // 800x600x15-bit
+            0x114 => (800, 600, 16, true),   // 800x600x16-bit
+            0x115 => (800, 600, 24, true),   // 800x600x24-bit
+            0x116 => (1024, 768, 15, true),  // 1024x768x15-bit
+            0x117 => (1024, 768, 16, true),  // 1024x768x16-bit
+            0x118 => (1024, 768, 24, true),  // 1024x768x24-bit
+            0x119 => (1280, 1024, 15, true), // 1280x1024x15-bit
+            0x11A => (1280, 1024, 16, true), // 1280x1024x16-bit
+            0x11B => (1280, 1024, 24, true), // 1280x1024x24-bit
+            _ => (0, 0, 0, false)


Magic numbers used in switch expression should be extracted to named constants in VbeConstants class. All VESA mode numbers (0x100-0x11B) should be defined as constants following the pattern established for VesaMode800x600x16. For example:

public const ushort VesaMode640x400x256 = 0x100; public const ushort VesaMode640x480x256 = 0x101; // etc.

This improves code maintainability and makes the mode numbers self-documenting.

src/Spice86.Core/Emulator/InterruptHandlers/VGA/VgaBios.cs

+            if (bpp == 15 || bpp == 16) {
+                modeInfo.RedMaskSize = VbeModeInfoConstants.RedGreenBlueMaskSize;
+                modeInfo.RedFieldPosition = (byte)(bpp == 16 ? 11 : 10);
+                modeInfo.GreenMaskSize = (byte)(bpp == 16 ? VbeModeInfoConstants.GreenMaskSize6Bit : VbeModeInfoConstants.RedGreenBlueMaskSize);


To fix this kind of issue, remove the redundant cast so that the code directly assigns the expression of the already-correct type. This keeps the code cleaner and avoids suggesting a nonexistent type conversion.

Concretely, in src/Spice86.Core/Emulator/InterruptHandlers/VGA/VgaBios.cs, within VbeGetModeInfo, update the line assigning modeInfo.GreenMaskSize. The ternary already produces a byte, so the explicit (byte) cast should be removed. The corrected line will simply assign the result of the conditional expression. No additional imports, methods, or definitions are required, and no other logic in the method needs to change.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.

Copilot · 2025-12-03T22:07:35Z

src/Spice86.Core/Emulator/InterruptHandlers/VGA/VgaBios.cs

+
+        // Fill VBE Info Block (VBE 1.0)
+        vbeInfo.Signature = "VESA";
+        vbeInfo.Version = VbeConstants.Version10;


The implementation reports VBE version 1.0 (0x0100) but the PR title, interface documentation (lines 10, 114), and comments throughout reference VBE 1.2 spec. If this is implementing VBE 1.2 functionality, the version should be 0x0102. If intentionally reporting 1.0 for compatibility while implementing 1.2 features, this should be clearly documented. Consider adding a constant Version12 = 0x0102 and either using it here or documenting why Version10 is used despite implementing 1.2 features.

Copilot · 2025-12-03T22:07:35Z

src/Spice86.Core/Emulator/InterruptHandlers/VGA/VgaBios.cs

+        if (bpp >= 15) {
+            if (bpp == 15 || bpp == 16) {
+                modeInfo.RedMaskSize = VbeModeInfoConstants.RedGreenBlueMaskSize;
+                modeInfo.RedFieldPosition = (byte)(bpp == 16 ? 11 : 10);


Magic number 10 at line 1532 should be extracted as a constant (e.g., RedFieldPositionRgb555 = 10) for better readability and maintainability.

Copilot · 2025-12-03T22:07:35Z

src/Spice86.Core/Emulator/InterruptHandlers/VGA/VgaBios.cs

+                modeInfo.RedFieldPosition = 16;
+                modeInfo.GreenMaskSize = VbeModeInfoConstants.RedGreenBlueMaskSize8;
+                modeInfo.GreenFieldPosition = 8;
+                modeInfo.BlueMaskSize = VbeModeInfoConstants.RedGreenBlueMaskSize8;
+                modeInfo.BlueFieldPosition = 0;


Magic numbers 16 and 8 at lines 1539 and 1541 should be extracted as constants (e.g., RedFieldPositionRgb24 = 16, GreenFieldPositionRgb24 = 8) for better readability and maintainability.

src/Spice86.Core/Emulator/InterruptHandlers/VGA/VgaBios.cs

Copilot · 2025-12-03T22:07:36Z

src/Spice86.Core/Emulator/InterruptHandlers/VGA/VgaBios.cs

+        if (bpp == 4) {
+            bytesPerLine = (ushort)(width / 8); // 4-bit planar
+        } else if (bpp == 1) {
+            bytesPerLine = (ushort)(width / 8);
+        } else if (bpp == 15 || bpp == 16) {
+            bytesPerLine = (ushort)(width * 2);
+        } else if (bpp == 24) {
+            bytesPerLine = (ushort)(width * 3);
+        } else if (bpp == 32) {
+            bytesPerLine = (ushort)(width * 4);
+        } else {
+            bytesPerLine = width; // 8-bit packed pixel
+        }


The bytes per scan line calculation logic (lines 1489-1501) uses multiple magic numbers (8, 2, 3, 4) that should be defined as named constants for clarity and maintainability. Consider extracting these as constants like BitsPerByte = 8, BytesPerPixel16Bit = 2, etc.

Copilot · 2025-12-03T22:07:36Z

src/Spice86.Core/Emulator/InterruptHandlers/VGA/VgaBios.cs

+        if (bpp >= 15) {
+            if (bpp == 15 || bpp == 16) {
+                modeInfo.RedMaskSize = VbeModeInfoConstants.RedGreenBlueMaskSize;
+                modeInfo.RedFieldPosition = (byte)(bpp == 16 ? 11 : 10);


Magic number 11 at line 1532 should be extracted as a constant (e.g., RedFieldPositionRgb565 = 11) for better readability and maintainability.

Copilot · 2025-12-03T22:07:36Z

src/Spice86.Core/Emulator/InterruptHandlers/VGA/VgaBios.cs

+                modeInfo.RedMaskSize = VbeModeInfoConstants.RedGreenBlueMaskSize;
+                modeInfo.RedFieldPosition = (byte)(bpp == 16 ? 11 : 10);
+                modeInfo.GreenMaskSize = (byte)(bpp == 16 ? VbeModeInfoConstants.GreenMaskSize6Bit : VbeModeInfoConstants.RedGreenBlueMaskSize);
+                modeInfo.GreenFieldPosition = 5;


Magic number 5 at line 1534 should be extracted as a constant (e.g., GreenFieldPositionRgb565 = 5) for better readability and maintainability.

maximilien-noal self-assigned this Dec 3, 2025

Copilot AI review requested due to automatic review settings December 3, 2025 21:45

Copilot started reviewing on behalf of maximilien-noal December 3, 2025 21:45 View session

Copilot finished reviewing on behalf of maximilien-noal December 3, 2025 21:48

Copilot AI reviewed Dec 3, 2025

View reviewed changes

github-advanced-security bot found potential problems Dec 3, 2025

View reviewed changes

maximilien-noal requested review from Copilot and kevinferrare December 3, 2025 22:02

Copilot started reviewing on behalf of maximilien-noal December 3, 2025 22:02 View session

Copilot finished reviewing on behalf of maximilien-noal December 3, 2025 22:06

Copilot AI reviewed Dec 3, 2025

View reviewed changes

maximilien-noal marked this pull request as draft December 4, 2025 06:52

maximilien-noal removed the request for review from kevinferrare December 6, 2025 21:59

maximilien-noal changed the title ~~feature: VESA VBE 1.2~~ [WIP] feature: VESA VBE 1.2 Dec 6, 2025

maximilien-noal force-pushed the feat/VBE_VESA_1.2 branch from 5aa17bf to 290c7b5 Compare December 24, 2025 20:35

maximilien-noal added the video Emulator video mode or video device related label Jan 13, 2026

maximilien-noal force-pushed the feat/VBE_VESA_1.2 branch 2 times, most recently from 30a8243 to 88734e1 Compare January 16, 2026 16:58

maximilien-noal force-pushed the feat/VBE_VESA_1.2 branch from 88734e1 to 1b5c29d Compare February 1, 2026 09:01

feature: VESA VBE 1.2

3ac5052

maximilien-noal force-pushed the feat/VBE_VESA_1.2 branch from 1b5c29d to 3ac5052 Compare February 3, 2026 18:19

	/// "The YCharSellSize...size of the character cell in pixels" = 16
	/// "The YCharCellSize...size of the character cell in pixels" = 16

Conversation

maximilien-noal commented Dec 3, 2025

Description of Changes

Rationale behind Changes

Suggested Testing Steps

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Check warning

Copilot Autofix

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants