NES: Improve OAM corruption accuracy.

Core/NES/BaseNesPpu.cpp

@@ -16,6 +16,7 @@ void BaseNesPpu::GetState(NesPpuState& state)
 	state.VideoRamAddr = _videoRamAddr;
 	state.TmpVideoRamAddr = _tmpVideoRamAddr;
 	state.SpriteRamAddr = _spriteRamAddr;
+	state.SecondaryOamAddr = _secondaryOamAddr;
 	state.ScrollX = _xScroll;
 	state.WriteToggle = _writeToggle;
 
@@ -40,6 +41,7 @@ void BaseNesPpu::SetState(NesPpuState& state)
 	_xScroll = state.ScrollX;
 	_writeToggle = state.WriteToggle;
 	_spriteRamAddr = state.SpriteRamAddr;
+	_secondaryOamAddr = state.SecondaryOamAddr;
 
 	_cycle = state.Cycle;
 	_scanline = state.Scanline;

Core/NES/BaseNesPpu.h

@@ -116,7 +116,6 @@ class BaseNesPpu : public INesMemoryHandler, public ISerializable
 	int32_t _openBusDecayStamp[8] = {};
 
 	uint64_t _oamDecayCycles[0x20] = {};
-	bool _corruptOamRow[32] = {};
 
 	bool IsRenderingEnabled();
 	void UpdateGrayscaleAndIntensifyBits();

Core/NES/NesPpu.cpp

@@ -56,9 +56,6 @@ template<class T> NesPpu<T>::NesPpu(NesConsole* console)
 		memcpy(_paletteRam, paletteRamBootValues, sizeof(_paletteRam));
 	}
 
-	//This should (presumably) persist across resets
-	memset(_corruptOamRow, 0, sizeof(_corruptOamRow));
-
 	//'v' is not cleared on reset, but it set to 0 on power on
 	_videoRamAddr = 0;
 
@@ -316,13 +313,10 @@ template<class T> uint8_t NesPpu<T>::PeekRam(uint16_t addr)
 		case PpuRegisters::SpriteData:
 			if(!_console->GetNesConfig().DisablePpu2004Reads) {
 				if(_scanline <= 239 && IsRenderingEnabled()) {
-					if(_cycle >= 257 && _cycle <= 320) {
-						uint8_t step = ((_cycle - 257) % 8) > 3 ? 3 : ((_cycle - 257) % 8);
-						uint8_t oamAddr = (_cycle - 257) / 8 * 4 + step;
-						returnValue = _secondarySpriteRam[oamAddr];
-					} else {
-						returnValue = _oamCopybuffer;
+					if((_cycle >= 257 && _cycle <= 340) || _cycle == 0) {
+						returnValue = _secondarySpriteRam[_secondaryOamAddr & 0x1F];
 					}
+					returnValue = _oamCopybuffer;
 				} else {
 					returnValue = _spriteRam[_spriteRamAddr];
 				}
@@ -368,19 +362,17 @@ template<class T> uint8_t NesPpu<T>::ReadRam(uint16_t addr)
 		case PpuRegisters::SpriteData:
 			if(!_console->GetNesConfig().DisablePpu2004Reads) {
 				if(_scanline <= 239 && IsRenderingEnabled()) {
-					//While the screen is begin drawn
-					if(_cycle >= 257 && _cycle <= 320) {
+					//While the screen is being drawn
+					if((_cycle >= 257 && _cycle <= 340) || _cycle == 0) {
 						//If we're doing sprite rendering, set OAM copy buffer to its proper value.  This is done here for performance.
 						//It's faster to only do this here when it's needed, rather than splitting LoadSpriteTileInfo() into an 8-step process
-						uint8_t step = ((_cycle - 257) % 8) > 3 ? 3 : ((_cycle - 257) % 8);
-						_secondaryOamAddr = (_cycle - 257) / 8 * 4 + step;
-						_oamCopybuffer = _secondarySpriteRam[_secondaryOamAddr];
+						_oamCopybuffer = _secondarySpriteRam[_secondaryOamAddr & 0x1F];
 					}
 					//Return the value that PPU is currently using for sprite evaluation/rendering
 					returnValue = _oamCopybuffer;
 				} else {
 					returnValue = ReadSpriteRam(_spriteRamAddr);
-					_emu->ProcessPpuWrite<CpuType::Nes>(_spriteRamAddr, returnValue, MemoryType::NesSpriteRam);
+					_emu->ProcessPpuRead<CpuType::Nes>(_spriteRamAddr, returnValue, MemoryType::NesSpriteRam);
 				}
 				openBusMask = 0x00;
 			}
@@ -447,6 +439,19 @@ template<class T> void NesPpu<T>::WriteRam(uint16_t addr, uint8_t value)
 			break;
 
 		case PpuRegisters::SpriteAddr:
+			//If OAM1ADDR is selected, updating the address can corrupt 2 rows (the open bus row and the destination row).
+			//This is actually alignment-dependent, but we're currently modeling worst-case behavior.
+			if(_settings->GetNesConfig().EnablePpuOamRowCorruption && _region == ConsoleRegion::Ntsc && (!_prevRenderingEnabled || _scanline >= 240 || (_cycle < 257 && (_cycle & 1)))) {
+				uint8_t sourceRow = _spriteRamAddr >> 3;
+				uint8_t openBusRow = _console->GetMemoryManager()->GetOpenBus() >> 3;
+				uint8_t destRow = value >> 3;
+				CorruptOamRow(sourceRow, openBusRow);
+				CorruptOamRow(openBusRow, destRow);
+			}
+			//TODO There is some kind of delay on committing the $2003 value on PAL (this is how it fixes the corruption issue).
+			//This currently isn't emulated.
+			//"In the 2C02, writes to $2003 took effect immediately. In the 2C07, they're buffered through pclk0+pclk1+pclk0[+pclk1+pclk0].
+			//I'm guessing that's what it took to prevent corruption. (it's actually pclk0+pclk1+pclk0 NOR pclk0+pclk1+pclk0+pclk1+pclk0)"
 			_spriteRamAddr = value;
 			break;
 
@@ -891,25 +896,20 @@ template<class T> void NesPpu<T>::ProcessScanlineImpl()
 
 		if(_scanline >= 0) {
 			((T*)this)->DrawPixel();
-			if(IsRenderingEnabled()) {
+			if(_prevRenderingEnabled) {
 				ShiftTileRegisters();
 			}
 			//"Secondary OAM clear and sprite evaluation do not occur on the pre-render line"
 			ProcessSpriteEvaluation();
-		} else if(_cycle < 9) {
+		} else if(_cycle == 1) {
 			//Pre-render scanline logic
-			if(_cycle == 1) {
-				_statusFlags.VerticalBlank = false;
-				_console->GetCpu()->ClearNmiFlag();
-			}
-			if(_spriteRamAddr >= 0x08 && IsRenderingEnabled() && !_settings->GetNesConfig().DisableOamAddrBug) {
-				//This should only be done if rendering is enabled (otherwise oam_stress test fails immediately)
-				//"If OAMADDR is not less than eight when rendering starts, the eight bytes starting at OAMADDR & 0xF8 are copied to the first eight bytes of OAM"
-				WriteSpriteRam(_cycle - 1, ReadSpriteRam((_spriteRamAddr & 0xF8) + _cycle - 1));
-			}
+			_statusFlags.VerticalBlank = false;
+			_console->GetCpu()->ClearNmiFlag();
 		}
 	} else if(_cycle >= 257 && _cycle <= 320) {
-		if(IsRenderingEnabled()) {
+		if(_prevRenderingEnabled) {
+			_sprite0Visible = _sprite0Added;
+
 			//"OAMADDR is set to 0 during each of ticks 257-320 (the sprite tile loading interval) of the pre-render and visible scanlines." (When rendering)
 			_spriteRamAddr = 0;
 
@@ -934,21 +934,28 @@ template<class T> void NesPpu<T>::ProcessScanlineImpl()
 			if(_cycle == 320) {
 				LoadExtraSprites();
 			}
+			//Increment OAM2ADDR during the first 4 dots of each set of 8. There is special behavior where the first set of 8 skips the
+			//first increment, and an extra increment is done after the last set of 8.
+			_secondaryOamAddr += !((_cycle - 1) & 4);
 		}
 		if(_cycle == 257) {
 			_spriteIndex = 0;
 			memset(_hasSprite, 0, sizeof(_hasSprite));
 			if(_prevRenderingEnabled) {
 				//copy horizontal scrolling value from t
 				_videoRamAddr = (_videoRamAddr & ~0x041F) | (_tmpVideoRamAddr & 0x041F);
+
+				//Fix up OAM2ADDR, which got incremented an extra time earlier. By doing this here, we avoid an extra if every dot.
+				_secondaryOamAddr--;
 			}
 		}
 	} else if(_cycle >= 321 && _cycle <= 336) {
 		LoadTileInfo();
 
 		if(_cycle == 321) {
-			if(IsRenderingEnabled()) {
-				_oamCopybuffer = _secondarySpriteRam[0];
+			if(_prevRenderingEnabled) {
+				//Do the final increment, which follows the last set of 8 sprite fetch dots.
+				_secondaryOamAddr++;
 			}
 		} else if(_prevRenderingEnabled && (_cycle == 328 || _cycle == 336)) {
 			_lowBitShift <<= 8;
@@ -990,8 +997,6 @@ template<class T> void NesPpu<T>::ProcessSpriteEvaluationStart()
 
 template<class T> void NesPpu<T>::ProcessSpriteEvaluationEnd()
 {
-	_sprite0Visible = _sprite0Added;
-
 	//Add 3 to address to count any partially-copied sprite.
 	//If eval is misaligned and wraps back to the start of OAM, the copy can
 	//be stopped mid-sprite (e.g only 1 to 3 bytes are copied to secondary OAM)
@@ -1011,15 +1016,19 @@ template<class T> void NesPpu<T>::ProcessSpriteEvaluationEnd()
 			_spriteCount++;
 		}
 	}
+
+	_secondaryOamAddr = 0;
 }
 
 template<class T> void NesPpu<T>::ProcessSpriteEvaluation()
 {
-	if(IsRenderingEnabled() || (_region == ConsoleRegion::Pal && _scanline >= _palSpriteEvalScanline)) {
+	if(_prevRenderingEnabled || (_region == ConsoleRegion::Pal && _scanline >= _palSpriteEvalScanline)) {
 		if(_cycle < 65) {
 			//Clear secondary OAM at between cycle 1 and 64
 			_oamCopybuffer = 0xFF;
-			_secondarySpriteRam[(_cycle - 1) >> 1] = 0xFF;
+			_secondarySpriteRam[_secondaryOamAddr & 0x1F] = 0xFF;
+
+			_secondaryOamAddr += !(_cycle & 1);
 		} else {
 			if(_cycle & 0x01) {
 				if(_cycle == 65) {
@@ -1029,9 +1038,9 @@ template<class T> void NesPpu<T>::ProcessSpriteEvaluation()
 				//Read a byte from the primary OAM on odd cycles
 				_oamCopybuffer = ReadSpriteRam(_spriteRamAddr);
 			} else {
-				if(_cycle == 256) {
-					ProcessSpriteEvaluationEnd();
-				}
+				//TODO These really should be local variables, because they're just a convenient way of handling the two parts of OAM1ADDR.
+				_spriteAddrH = _spriteRamAddr >> 2;
+				_spriteAddrL = _spriteRamAddr & 3;
 
 				if(_oamCopyDone && !_settings->GetNesConfig().EnablePpuSpriteEvalBug) {
 					_spriteAddrH = (_spriteAddrH + 1) & 0x3F;
@@ -1069,6 +1078,7 @@ template<class T> void NesPpu<T>::ProcessSpriteEvaluation()
 							//Note: Using "(_secondaryOamAddr & 0x03) == 0" instead of "_spriteAddrL == 0" is required
 							//to replicate a hardware bug noticed in oam_flicker_test_reenable when disabling & re-enabling
 							//rendering on a single scanline
+							//TODO This should actually be using a timer (which runs even with rendering off) rather than the OAM2 address.
 							if((_secondaryOamAddr & 0x03) == 0) {
 								//Done copying all 4 bytes
 								_spriteInRange = false;
@@ -1134,6 +1144,9 @@ template<class T> void NesPpu<T>::ProcessSpriteEvaluation()
 					}
 				}
 				_spriteRamAddr = (_spriteAddrL & 0x03) | (_spriteAddrH << 2);
+				if(_cycle == 256) {
+					ProcessSpriteEvaluationEnd();
+				}
 			}
 		}
 	}
@@ -1279,49 +1292,6 @@ template<class T> void NesPpu<T>::DebugUpdateFrameBuffer(bool toGrayscale)
 	}
 }
 
-template<class T> void NesPpu<T>::SetOamCorruptionFlags()
-{
-	if(!_console->GetNesConfig().EnablePpuOamRowCorruption) {
-		return;
-	}
-
-	//Note: Still pending more research, but this currently matches a portion of the issues that have been observed
-	//When rendering is disabled in some sections of the screen, either:
-	// A- During Secondary OAM clear (first ~64 cycles)
-	// B- During OAM tile fetching (cycle ~256 to cycle ~320)
-	//then OAM memory gets corrupted the next time the PPU starts rendering again (usually at the start of the next frame)
-	//This usually causes the first "row" of OAM (the first 8 bytes) to get copied over another, causing some sprites to be missing
-	//and causing an extra set of the first 2 sprites to appear on the screen (not possible to see them except via any overflow they may cause)
-
-	if(_cycle >= 0 && _cycle < 64) {
-		//Every 2 dots causes the corruption to shift down 1 OAM row (8 bytes)
-		_corruptOamRow[_cycle >> 1] = true;
-	} else if(_cycle >= 256 && _cycle < 320) {
-		//This section is in 8-dot segments.
-		//The first 3 dot increment the corrupted row by 1, and then the last 5 dots corrupt the next row for 5 dots.
-		uint8_t base = (_cycle - 256) >> 3;
-		uint8_t offset = std::min<uint8_t>(3, (_cycle - 256) & 0x07);
-		_corruptOamRow[base * 4 + offset] = true;
-	}
-}
-
-template<class T> void NesPpu<T>::ProcessOamCorruption()
-{
-	if(!_console->GetNesConfig().EnablePpuOamRowCorruption) {
-		return;
-	}
-
-	//Copy first OAM row over another row, as needed by corruption flags (can be over itself, which causes no actual harm)
-	for(int i = 0; i < 32; i++) {
-		if(_corruptOamRow[i]) {
-			if(i > 0) {
-				memcpy(_spriteRam + i * 8, _spriteRam, 8);
-			}
-			_corruptOamRow[i] = false;
-		}
-	}
-}
-
 template<class T> void NesPpu<T>::Exec()
 {
 	if(_cycle < 340) {
@@ -1369,10 +1339,6 @@ template<class T> void NesPpu<T>::ProcessScanlineFirstCycle()
 		//Force prerender scanline sprite fetches to load the dummy $FF tiles (fixes shaking in Ninja Gaiden 3 stage 1 after beating boss)
 		_spriteCount = 0;
 
-		if(_renderingEnabled) {
-			ProcessOamCorruption();
-		}
-
 		_emu->ProcessEvent(EventType::StartFrame);
 
 		UpdateMinimumDrawCycles();
@@ -1392,17 +1358,27 @@ template<class T> void NesPpu<T>::ProcessScanlineFirstCycle()
 			_statusFlags.Sprite0Hit = false;
 			_allowFullPpuAccess = true;
 
+			//2C02E+ bug where OAM reliably corrupts if the row changes at the start of pre-render. Required for Huge Insect.
+			if(IsRenderingEnabled() && !_settings->GetNesConfig().DisableOamAddrBug) {
+				CorruptOamRow(_spriteRamAddr >> 3, _secondaryOamAddr & 0x1F);
+			}
+
 			//Switch to alternate output buffer (VideoDecoder may still be decoding the last frame buffer)
 			_currentOutputBuffer = (_currentOutputBuffer == _outputBuffers[0]) ? _outputBuffers[1] : _outputBuffers[0];
 			_emu->AddDebugEvent<CpuType::Nes>(DebugEventType::BgColorChange);
 		} else if(_prevRenderingEnabled) {
+			_secondaryOamAddr = 0;
+
 			if(_scanline > 0 || (!(_frameCount & 0x01) || _region != ConsoleRegion::Ntsc || GetPpuModel() != PpuModel::Ppu2C02)) {
 				//Set bus address to the tile address calculated from the unused NT fetches at the end of the previous scanline
 				//This doesn't happen on scanline 0 if the last dot of the previous frame was skipped
 				SetBusAddress((_tile.TileAddr << 4) | (_videoRamAddr >> 12) | _control.BackgroundPatternAddr);
 			}
 		}
 	} else if(_scanline == 240) {
+		if(_prevRenderingEnabled) {
+			_secondaryOamAddr = 0;
+		}
 		//At the start of vblank, the bus address is set back to VideoRamAddr.
 		//According to Visual NES, this occurs on scanline 240, cycle 1, but is done here on cycle 0 for performance reasons
 		SetBusAddress(_videoRamAddr & 0x3FFF);
@@ -1418,6 +1394,17 @@ template<class T> void NesPpu<T>::ProcessScanlineFirstCycle()
 	}
 }
 
+template<class T> void NesPpu<T>::CorruptOamRow(uint8_t sourceRow, uint8_t destRow)
+{
+	if(sourceRow != destRow) {
+		uint8_t sourceByte = (sourceRow << 3) & 0xFF;
+		uint8_t destByte = (destRow << 3) & 0xFF;
+
+		memcpy(_spriteRam + destByte, _spriteRam + sourceByte, 8);
+		_secondarySpriteRam[destRow] = _secondarySpriteRam[sourceRow];
+	}
+}
+
 template<class T> void NesPpu<T>::UpdateState()
 {
 	_needStateUpdate = false;
@@ -1427,13 +1414,15 @@ template<class T> void NesPpu<T>::UpdateState()
 		_emu->AddDebugEvent<CpuType::Nes>(DebugEventType::BgColorChange);
 		_prevRenderingEnabled = _renderingEnabled;
 		if(_scanline < 240) {
-			if(_prevRenderingEnabled) {
-				//Rendering was just enabled, perform oam corruption if any is pending
-				ProcessOamCorruption();
-			} else if(!_prevRenderingEnabled) {
-				//Rendering was just disabled by a write to $2001, check for oam row corruption glitch
-				SetOamCorruptionFlags();
+			if(_settings->GetNesConfig().EnablePpuOamRowCorruption && (_cycle >= 257 || (_cycle & 1) == 0)) {
+				if(_renderingEnabled) {
+					CorruptOamRow(_spriteRamAddr >> 3, _secondaryOamAddr & 0x1F); //OAM1 row to OAM2 row.
+				} else {
+					CorruptOamRow(_secondaryOamAddr & 0x1F, _spriteRamAddr >> 3); //OAM2 row to OAM1 row.
+				}
+			}
 
+			if(!_prevRenderingEnabled) {
 				//When rendering is disabled midscreen, set the vram bus back to the value of 'v'
 				SetBusAddress(_videoRamAddr & 0x3FFF);
 
@@ -1444,12 +1433,6 @@ template<class T> void NesPpu<T>::UpdateState()
 					//     if rendering is disabled on an odd cycle, the increment will wait until the next odd cycle (at which point it will be incremented by 1)
 					//In practice, there is no way to see the difference, so we just increment by 1 at the end of the next cycle after rendering was disabled
 					_spriteRamAddr++;
-
-					//Also corrupt H/L to replicate a bug found in oam_flicker_test_reenable when rendering is disabled around scanlines 128-136
-					//Reenabling the causes the OAM evaluation to restart misaligned, and ends up generating a single sprite that's offset by 1
-					//such that it's Y=tile index, index = attributes, attributes = x, and X = the next sprite's Y value
-					_spriteAddrH = (_spriteRamAddr >> 2) & 0x3F;
-					_spriteAddrL = _spriteRamAddr & 0x03;
 				}
 			}
 		}
@@ -1645,8 +1628,6 @@ template<class T> void NesPpu<T>::Serialize(Serializer& s)
 			_oamDecayCycles[i] = _console->GetCpu()->GetCycleCount();
 		}
 
-		memset(_corruptOamRow, 0, sizeof(_corruptOamRow));
-
 		for(int i = 0; i < 257; i++) {
 			_hasSprite[i] = true;
 		}

Core/NES/NesPpu.h

@@ -70,8 +70,7 @@ class NesPpu : public BaseNesPpu
 	__forceinline uint8_t ReadSpriteRam(uint8_t addr);
 	__forceinline void WriteSpriteRam(uint8_t addr, uint8_t value);
 
-	void SetOamCorruptionFlags();
-	void ProcessOamCorruption();
+	__forceinline void CorruptOamRow(uint8_t sourceRow, uint8_t destRow);
 
 	__forceinline uint8_t GetPixelColor();
 

Core/NES/NesTypes.h

@@ -258,6 +258,7 @@ struct NesPpuState : public BaseState
 	uint8_t ScrollX;
 	bool WriteToggle;
 	uint8_t SpriteRamAddr;
+	uint8_t SecondaryOamAddr;
 };
 
 struct ApuLengthCounterState

UI/Debugger/RegisterViewer/NesRegisterViewer.cs

@@ -58,7 +58,8 @@ private static RegisterViewerTab GetNesPpuTab(ref NesState state)
 			new RegEntry("$2002.6", "Sprite 0 hit", ppu.StatusFlags.Sprite0Hit),
 			new RegEntry("$2002.7", "Vertical blank", ppu.StatusFlags.VerticalBlank),
 
-			new RegEntry("$2003", "OAM address", ppu.SpriteRamAddr, Format.X8),
+			new RegEntry("$2003", "OAM1 address", ppu.SpriteRamAddr, Format.X8),
+			new RegEntry("", "OAM2 address", ppu.SecondaryOamAddr & 0x1F, Format.X8),
 
 			new RegEntry("$2005-2006", "VRAM Address / Scrolling"),
 			new RegEntry("", "VRAM Address", ppu.VideoRamAddr, Format.X16),