| H@P |
|
Lurker in perpetuity
|
|
|
|
|
Reged: 09/22/03
|
|
Posts: 234
|
|
Loc: Seattle area
|
|
|
|
Send PM
|
|
|
Aaaargh!
#358387 - 09/07/16 03:43 AM
|
|
|
Not the game. This is about Carnival. Rant follows.
So I showed the game to my son, but something seemed off. It seemed I had cleared the level but it wouldn't end. Then more ducks started coming out. While randomly shooting, I hit something invisible and I was able to complete the level.
I compile my own linux builds, and have each major version archived. I found 0.152 was fine (white rabbits! it was missing all white rabbits!), 0.153 was broken. I couldn't find anything touching the vicdual files. So I ran a git bisect and tracked down the offending commit. I later saw I was searching for vicdual.cpp changes, and it was still *.c at that point... oops.
When the const rgb_t::white is used in a static global palette array, the const doesn't seem to have been initialized yet (runtime initialization of the const value in the class?) so it actually stuffs a black palette entry in its place (uninitialized in this case is the same as black). So in Carnival, rabbits were black on a black background, hence invisible. I confirmed when used in a function directly, or even when used in the same palette array, but defined as a static local, rgb_t::white works as expected.
I checked if other drivers were affected, and found a few. Tested each before, made the fix, tested again, and things lit up white like expected.
So I go to test these changes on a windows pc, and Carnival works just fine in stock 0.177. What the hell?!?
Pretty sure I'm using a fairly recent gcc on both linux and windows.
Can anyone else on linux confirm Carnival is missing white rabbits on 0.177?
What is happening? =(X^@
H@P
Edited by H@P (09/07/16 03:57 AM)
|
|
|
|
Re: Aaaargh!
[Re: H@P]
#358388 - 09/07/16 07:03 AM
|
|
|
If it's something that depends on the order of static initialisation across files, then really it's a legitimate bug that needs to be fixed. But I don't quite understand the issue as you described, the text meandered, so I'm not sure what you're saying.
|
|
|
|
Re: Aaaargh!
[Re: Vas Crabb]
#358391 - 09/07/16 09:58 AM
|
|
|
> If it's something that depends on the order of static initialisation across files,
> then really it's a legitimate bug that needs to be fixed. But I don't quite
> understand the issue as you described, the text meandered, so I'm not sure what
> you're saying.
For H@P, one of the rows of things to shoot was missing in the game Carnival, using a self-compile on Linux of the 0.177 tag of MAME. He then checked builds circa 0.152, and the bug wasn't there. Upon digging further, he found a number of affected games, not just Carnival or vicdual-driver games.
He traced the regression to something to do with how rgb_t is initialized when const, but then when attempting to reproduce the bug before digging deeper on a Windows machine, he found that the bug didn't happen on Windows.
He wonders, then, if this is an actual code issue with MAME, or if it's an issue with gcc, and if it is, whether or not it's platform-specific.
|
|
|
| casm |
|
Cinematronics > *
|
|
|
|
|
Reged: 08/27/07
|
|
Posts: 668
|
|
|
|
|
|
Send PM
|
|
|
Re: Aaaargh!
[Re: H@P]
#358408 - 09/07/16 03:12 PM
|
|
|
> Can anyone else on linux confirm Carnival is missing white rabbits on 0.177?
Added for point of comparison: hand-built 0.177 on OS X 10.10.5 is fine.
Granted, this build is done using clang (Apple LLVM version 7.0.2 (clang-700.1.81)) and not gcc, but is probably representative of what most OS X users would be building under.
|
|
|
| R. Belmont |
|
Cuckoo for IGAvania
|
|
|
|
|
|
Reged: 09/21/03
|
|
Posts: 9716
|
|
Loc: ECV-197 The Orville
|
|
|
|
Send PM
|
|
|
Re: Aaaargh!
[Re: Vas Crabb]
#358409 - 09/07/16 03:18 PM
|
|
|
> If it's something that depends on the order of static initialisation across files,
> then really it's a legitimate bug that needs to be fixed. But I don't quite
> understand the issue as you described, the text meandered, so I'm not sure what
> you're saying.
The stock colors are initialized after global statics are constructed, so this:
static const rgb_t colors[] = { rgb_t::white };
will yield a random (usually black) color, unless it's inside a function's scope. This has been a known thing for at least 2 years now. The stock colors really should be #defines or something to prevent this. I had to change the Apple II drivers to just use hex RGB values instead.
Edited by R. Belmont (09/07/16 03:19 PM)
|
|
|
| AWJ |
|
|
|
|
|
|
|
Reged: 03/08/05
|
|
Posts: 936
|
|
Loc: Ottawa, Ontario
|
|
|
|
Send PM
|
|
|
Re: Aaaargh!
[Re: R. Belmont]
#358413 - 09/07/16 06:01 PM
|
|
|
> The stock colors are initialized after global statics are constructed, so this:
>
> static const rgb_t colors[] = { rgb_t::white };
>
> will yield a random (usually black) color, unless it's inside a function's scope.
> This has been a known thing for at least 2 years now. The stock colors really should
> be #defines or something to prevent this. I had to change the Apple II drivers to
> just use hex RGB values instead.
Now that MAME is C++14, shouldn't this be fixable by declaring the rgb_t's constructor(s) constexpr? (not 100% sure about this, I've read conflicting things about exactly what constexpr does...)
|
|
|
|
Re: Aaaargh!
[Re: Vas Crabb]
#358419 - 09/07/16 07:49 PM
|
|
|
Unfortunately, it does depend on static initialization. In src/mame/video/vicdual.cpp can be found this hard-coded color table:
Code:
static const pen_t pens_from_color_prom[] =
{
rgb_t::black,
rgb_t(0x00, 0xff, 0x00),
rgb_t(0x00, 0x00, 0xff),
rgb_t(0x00, 0xff, 0xff),
rgb_t(0xff, 0x00, 0x00),
rgb_t(0xff, 0xff, 0x00),
rgb_t(0xff, 0x00, 0xff),
rgb_t::white
};
rgb_t::black, rgb_t::white and a bunch of similar variables are declared as static const in src/lib/util/palette.h.
|
|
|
|
Re: Aaaargh!
[Re: AWJ]
#358420 - 09/07/16 08:27 PM
|
|
|
> Now that MAME is C++14, shouldn't this be fixable by declaring the rgb_t's
> constructor(s) constexpr? (not 100% sure about this, I've read conflicting things
> about exactly what constexpr does...)
Yes, but GCC has some known issues with constexpr variables that may interfere with this plan.
|
|
|
| H@P |
|
Lurker in perpetuity
|
|
|
|
|
|
Reged: 09/22/03
|
|
Posts: 234
|
|
Loc: Seattle area
|
|
|
|
Send PM
|
|
|
Re: Aaaargh!
[Re: H@P]
#359223 - 10/02/16 12:29 AM
|
|
|
Finally got a chance to poke around at this again.
Noticed Windows gcc was using 6.x while Linux was using 5.x. Added 6.x as a gcc alternative, clean built, and the problem still persists.
Removed all static const colors:
- // constants
- static const rgb_t black;
- static const rgb_t white;
- static const rgb_t green;
- static const rgb_t amber;
- static const rgb_t transparent;
and replaced with #defines:
+#define RGB_T__BLACK rgb_t(0, 0, 0)
+#define RGB_T__WHITE rgb_t(255, 255, 255)
+#define RGB_T__GREEN rgb_t(0, 255, 0)
+#define RGB_T__AMBER rgb_t(247, 170, 0)
+#define RGB_T__TRANSPARENT rgb_t(0, 0, 0, 0)
and all is well.
Let me know if this would be acceptable to submit.
Or is it better to just add the literal initialization everywhere, e.g. either this
static const pen_t pens_from_color_prom[] =
{
- rgb_t::black,
+ RGB_T__BLACK,
rgb_t(0x00, 0xff, 0x00),
rgb_t(0x00, 0x00, 0xff),
rgb_t(0x00, 0xff, 0xff),
rgb_t(0xff, 0x00, 0x00),
rgb_t(0xff, 0xff, 0x00),
rgb_t(0xff, 0x00, 0xff),
- rgb_t::white
+ RGB_T__WHITE
};
or this:
static const pen_t pens_from_color_prom[] =
{
- rgb_t::black,
+ rgb_t(0x00, 0x00, 0x00),
rgb_t(0x00, 0xff, 0x00),
rgb_t(0x00, 0x00, 0xff),
rgb_t(0x00, 0xff, 0xff),
rgb_t(0xff, 0x00, 0x00),
rgb_t(0xff, 0xff, 0x00),
rgb_t(0xff, 0x00, 0xff),
- rgb_t::white
+ rgb_t(0xff, 0xff, 0xff)
};
Your code base, you tell me which is more acceptable, if either...
H@P
Edited by H@P (10/02/16 01:06 AM)
|
|
|
|
Re: Aaaargh!
[Re: H@P]
#359240 - 10/02/16 12:36 PM
|
|
|
Anything containing double underscore is out, as those are reserved. Might be better to add static methods to rgb_t to return the colours rather than the macro stuff.
Code:
class rgb_t
{
...
static constexpr rgb_t black() { return rgb_t(0, 0, 0); }
static constexpr rgb_t white() { return rgb_t(255, 255, 255); }
...
};
Not much can go wrong with doing it that way.
|
|
|
|
Index 
Threaded 
Aaaargh!
Reply
[Re: 
