I was just noticing an issue with the NTSC filter in Nestopia.
Attachment:
super_mario_bros_title_screen_nestopia.png [ 13.12 KiB | Viewed 3874 times ]
If you look at the screenshot, there is a color transition edge on the left and right sides of the screen.
However, this should not actually exist, since there should be a little bit of overscan on the left and right sides of the image. The background color should continue a little longer, and should not have any color change at the edge of the screen.
Dwedit wrote:
I was just noticing an issue with the NTSC filter in Nestopia.
Attachment:
super_mario_bros_title_screen_nestopia.png
If you look at the screenshot, there is a color transition edge on the left and right sides of the screen.
However, this should not actually exist, since there should be a little bit of overscan on the left and right sides of the image. The background color should continue a little longer, and should not have any color change at the edge of the screen.
Sega Li can display background color and its NTSC filter looks great ...
but it can't use scanlines..
This is because of the instances of lut.black in ./source/core/NstVideoFilterNtsc.cpp. If they were replaced by the appropriate palette numbers, it would DTRT. Unfortuantely, the C++ goo is all over nestopia and I haven't least idea how to properly violate the abstraction to get the correct palette data.
I attempted to get through the abstractions to pass the information from the palette to the NTSC filter. I added three bgColor members to let it trickle down. It looks like it could also have been passed partially through the Blit() call, as the burstPhase is. But adding a parameter to Blit() would have involved changing all the renderers' Blit() functions. I don't have the Linux version to try compiling, so you'll have to try these changes to the source.
This is kind of hacky, and still isn't enough. Ideally, Nestopia would pad the left and right sides of the raw PPU output with zero-pixel colors, so that the renderer could show some overscan (if desired) on the left and right sides, and get even more of the background color. As it is, it just gets a few pixels of it (that are mostly cut off) as necessary to render the pixels properly in NTSC mode.
> I attempted to get through the abstractions to pass the information from the palette to the NTSC filter. I added three bgColor members to let it trickle down.
Hah, that reminds me of working with libgambatte. I want to get the Y-counter for the LCD renderer, that the SGB emulation needs. LCDC is a private class that doesn't expose it. Itself only a private member of the LCD class. The LCD class is only a private member of the Memory class (which doesn't really make sense, but whatever.) The Memory class is only a private member of the CPU class. The CPU class is only a private member of the GambatteCore class. The GambatteCore class is only a private member of the libgambatte interface class.
The actual Y-counter value was -extremely- well guarded from the outside world. I'd go crazy trying to write a debugger like that.
With my code, I made up a keyword called privileged. When you compile with -DDEBUGGER (which has no speed penalty), it becomes public. When you compile without that, it becomes private. This helps keep the interface clean and guarded, and also lets you get at the data you want when you absolutely need it.
It also helps to design the core where you have system{CPU, APU, PPU, Memory} rather than system{CPU}->{APU}->{PPU}->{Memory}. All one level deep rather than all nested inside each other.
I did something like that too. The value in access control is enforced restrictions on users, making modifications easier since you can better predict the consequences of a change to the class. By being able to enforce this for normal builds, you get that value, but can then take it away where full access is desired. If friendship were transitive, you could just make a base class a friend and derive from that for special access. Nested classes are tedious to deal with as well; better to put them into a side namespace and include its header as needed.
Surely there's a balance between overly layered systems and everything-is-a-global ones that are a nightmare to modify without breaking (but easy to write debuggers for). Both suffer from major resistance to certain kinds of changes, a kind of momentum that ties the hands of maintainers in different ways.
A huge advantage would be like you said, generic friends.
People love real life analogies with computing, so they'll see "a friend of a friend may not be your friend", and that "only the class itself can declare who its friends are", sure, but when it comes to programming, it'd be nice to have something like a debugger class that says it is friends with your emulator core classes. Or a single Emulator::Debugger friend class you could inherit from and get the desired access. The users knows when they declare these things that it may break in a future library revision. Private access is trivial to bypass anyway (byuu.org/articles/programming/public_cast)
Easy way to do the latter would be to extend the keyword. friend == private friend, and then add protected friend and public friend.
At this point, I want subclasses so badly I'm seriously contemplating a custom preprocessor
blargg wrote:
I attempted to get through the abstractions to pass the information from the palette to the NTSC filter. I added three bgColor members to let it trickle down. It looks like it could also have been passed partially through the Blit() call, as the burstPhase is. But adding a parameter to Blit() would have involved changing all the renderers' Blit() functions. I don't have the Linux version to try compiling, so you'll have to try these changes to the source.
This is kind of hacky, and still isn't enough. Ideally, Nestopia would pad the left and right sides of the raw PPU output with zero-pixel colors, so that the renderer could show some overscan (if desired) on the left and right sides, and get even more of the background color. As it is, it just gets a few pixels of it (that are mostly cut off) as necessary to render the pixels properly in NTSC mode.
Thanks for taking a look at this, i tried to compile the changes but ended up with errors for some reason but i will try again.
What errors? I didn't compile my changes so it's unlikely they would work without minor errors.
blargg wrote:
What errors? I didn't compile my changes so it's unlikely they would work without minor errors.
Sorry blargg i meant i came up with compiling errors about subclass i believe when i tried to compile the changes. Does the code changes in effect pretty much try to do this?:
http://img40.imageshack.us/img40/8381/1nesg.jpgThis pic is of the actual nes, you can see the blue sky continues on the right side of the screen.
Yes, it is to set the background that Nestopia uses to the NES's BG color, rather than black as Nestopia currently uses.
blargg wrote:
Yes, it is to set the background that Nestopia uses to the NES's BG color, rather than black as Nestopia currently uses.
Ok thanks ill try to add the changes you posted again and see how it goes. Perhaps i did not do something correctly the first time.
Blargg's patch does not build cleanly (he says so, I've tested). I fought with it for a bit and then gave up...
ok i tried it again and get these errors:
error c2065: 'i' undeclared identifier: NstPPu.Cpp
error c2248: NstMachine.cpp : cannot access private member declared in class ::NES::CORE:PPU
error c2065: bgcolor undeclared identifier NstVideoRenderer
error c2039: bgcolor is not a member of NES::CORE::VIDEO:RENDERER
i made a pastbin of the ntsc.cpp file because i think i messed up something but those are the main errors:
http://pastebin.com/fq8E4F0VEDIT: ok i see the post before, nevermind lol guess i missed where that was said lol. Ill wait and see if anything else pops up from anyone who might be able to help fix the bug.
OK, I saw that I had since successfully built Nestopia from source on my machine, and got this patch working. I used diff -c6 -r, which I hope allows simple patching (I can re-diff if there's a simpler way). (weird, the sky color looks too purple below, but clicking on the picture shows it right. I'm using Firefox latest version)
[see later post for fixed patch]
Thanks for posting the fix blargg. Looks good. Most of my errors went away.
The only errors im getting are in NstVideoFilterNTSC.cpp
im getting a bunch of syntax errors. I put up a pastebin cause im pretty sure i did this right:
http://pastebin.com/KKw2xgyEIll go back through and check again, the only confusing part was the bits part in this cpp file.
ok now im confused here im down to one error now:
error c2248: NES::CORE:PPU:OUTPUT cannot access private member
this line of code : renderer.bgColor = ppu.output.bgColor;
I'm not sure how you're getting any errors with the patch I posted. That compiled without any errors when I issued make. I'm using Ubuntu 12.04 updated a few days ago.
blargg wrote:
I'm not sure how you're getting any errors with the patch I posted. That compiled without any errors when I issued make. I'm using Ubuntu 12.04 updated a few days ago.
Everything is fine until i put this line in: renderer.bgColor = ppu.output.bgColor; in NstMachine.cpp It always says:
error C2248: 'Nes::Core::Ppu::output' : cannot access private member declared in class 'Nes::Core::Ppu'
Im using vs.net 2003 as im adding this into the xbox port. But i dont think it matters what compiler im using does it?
lol one more thing, if i change it to:
#pragma renderer.bgColor = ppu.output.bgColor;
or
#pragma warning renderer.bgColor = ppu.output.bgColor;
then it compiles..but i dont think that will work though.
Did you make bgColor a public member of NstVideoRenderer.hpp?
I followed your list to a t. this is what i added:
virtual ~Filter() {}
virtual void Blit(const Input&,const Output&,uint) = 0;
virtual void Transform(const byte (&)[PALETTE][3],Input::Palette&) const;
const Format format;
+
+ uint bgColor;
};
struct State
{
State();
***************
*** 234,245 ****
--- 237,249 ----
Filter* filter;
State state;
Palette palette;
public:
+ uint bgColor;
Result SetBrightness(int brightness)
{
return SetLevel( state.brightness, brightness );
}
But if it compiles with a #pragma, im not sure why it would with it but thats the only way i can get it to. Am i missing anything else?
I think that the #pragma is basically commenting it out. #pragma whatever is a compiler directive that is handled specifically to the compiler.
And wow, sorry, apparently the makefile isn't setup up right or make just doesn't work at all like I thought. When I modify e.g. NstVideoRenderer.hpp and issue make, nothing is recompiled. So I need to apparently do make clean to even be sure it compiles changed files. Ugh.
OK, one addition:
Code:
diff -c6 -r source-orig/core/NstPpu.hpp source/core/NstPpu.hpp
*** source-orig/core/NstPpu.hpp 2012-11-28 15:23:14.000000000 -0600
--- source/core/NstPpu.hpp 2013-03-18 20:36:39.582196143 -0500
***************
*** 409,421 ****
--- 410,424 ----
Regs regs;
Scroll scroll;
Tiles tiles;
Chr chr;
Nmt nmt;
int scanline;
+ public:
Output output;
+ private:
PpuModel model;
Hook hActiveHook;
Hook hBlankHook;
const byte* rgbMap;
const byte* yuvMap;
Oam oam;
thanks that worked, could you do me a favor and attach what your NstVideoFilterNtsc.cpp file looks like?
The reason why i ask is becuase i have in my code the scanline xbox stuff at this point:
for (uint y=HEIGHT; y; --y)
}
NES_NTSC_BEGIN_ROW( &lut, phase, bgcolor, bgcolor, *src++ );
Pixel temp;
// 50%
#define PIXEL_OUT( i ) \
NES_NTSC_RGB_OUT( i, temp, BITS );\
dst[i] = temp;\
temp >>= 1;\
temp &= 0x7BEF;\
reinterpret_cast<Pixel*>(reinterpret_cast<char*>(dst)+pitch)[i] = temp;
and want to make sure i have everything in the right place. Since some of this code starts right where we added the scanlines stuff the other day.
At some point in the future is there a possibility to be able to extend the overscan on the sides a little bit more with the ntsc filter? Not sure how easy/hard it would be.
*Spitfire_NES* wrote:
At some point in the future is there a possibility to be able to extend the overscan on the sides a little bit more with the ntsc filter? Not sure how easy/hard it would be.
Only semi-related, but does anybody know if the background colored "borders" (that I assume you're referring to) are affected by pointing PPU address in the palette area while rendering is disabled?
Yes, they are; my flowing palette demo paints well into horizontal overscan.
Is there a way in the future to extend the sides a little more?