Josh @ Dreamland
|
|
Posted on: June 15, 2013, 07:29:33 am |
|
|
Prince of all Goldfish
Location: Pittsburgh, PA, USA Joined: Feb 2008
Posts: 2950
|
Two quick items:
(1) Your "fix" for arbitrary dot-access variables isn't working. I've had fifty people reporting segfaults in the compiler recently, and Frogg reported that [snip=edl]someobject.unusedvariable[0][/snip] is the same object for any [snip]unusedvariable[/snip]. So basically, your fix apparently doesn't work, and in development teams, correlation usually does imply causation, so I'm guessing the segfault is from your shit.
(2) You do this practically everywhere:
enigma::tile t = dit->second.tiles[i]; t.alpha = 1;
It doesn't do jack shit without an & after enigma::tile, or enigma::room_background, or any of the other fifty places you do it.
...
Okay, I'll just start listing things.
(3) Does your fucking tile system assign an alpha to every single tile, but use it exclusively for tile_layer_show/hide? Is it actually drawing hundreds or thousands of completely transparent tiles when a layer is hidden?
I won't even blame you for that, because at least you implemented it. But holy shit.
|
|
« Last Edit: June 15, 2013, 07:33:27 am by Josh @ Dreamland »
|
Logged
|
"That is the single most cryptic piece of code I have ever seen." -Master PobbleWobble "I disapprove of what you say, but I will defend to the death your right to say it." -Evelyn Beatrice Hall, Friends of Voltaire
|
|
|
polygone
|
|
Reply #1 Posted on: June 15, 2013, 07:52:30 am |
|
|
Location: England Joined: Mar 2009
Posts: 794
|
I've had fifty people reporting segfaults in the compiler recently
Maybe you could tell me what is actually causing the segfaults specifically? Frogg reported that [snip=edl]someobject.unusedvariable[0][/snip] is the same object for any [snip]unusedvariable[/snip].
I don't see how that would be the case? Why would it be handled differently to a variable.. enigma::tile t = dit->second.tiles[i]; t.alpha = 1;
What has this got to do with alpha? All the variables are set like so (I think) so if that's not working then nothing would be... Does your fucking tile system assign an alpha to every single tile, but use it exclusively for tile_layer_show/hide? Is it actually drawing hundreds or thousands of completely transparent tiles when a layer is hidden?
I won't even blame you for that, because at least you implemented it. But holy shit.
Yes, that's what it does. Setting alpha to 0 is used for hiding single tiles but I don't think it's done like that for layers. But at the moment I'm too lazy to actually look at any of this myself and check any of it, so I'm just going on memory.
|
|
|
Logged
|
I honestly don't know wtf I'm talking about but hopefully I can muddle my way through.
|
|
|
forthevin
|
|
Reply #2 Posted on: June 15, 2013, 02:34:36 pm |
|
|
Joined: Jun 2012
Posts: 167
|
In regards to the issue with the tiles and copying of structs, most of the time the direct assignment is actually correct, since polygone uses it to either just read from the tile, or copy it in again, either after modification of the tile or after the tile in the data structure being deleted.
That said, there are a couple of functions where it indeed is not used correctly, such as [snip]bool tile_layer_hide(int layer_depth);[/snip], [snip]bool tile_layer_show(int layer_depth);[/snip] and [snip]bool tile_layer_shift(int layer_depth, int x, int y);[/snip]. In those cases, the tile struct is copied, the copy is modified, and nothing more is done.
I also think I found a somewhat unrelated bug, namely in [snip]tile_layer_delete_at[/snip]. The issue is that a vector is iterated through (in this case using indexes) while it is being modified (namely, elements are removed from it), and I think in this case it results in that sometimes, elements that should have been removed are not removed. Modifying a data structure while iterating through it can easily be gotten wrong, so great care should always be taken when doing so - or, alternatively, simply avoid iterating through it while modifying it, for instance by creating a temporary vector or the like (though that may not always be performant).
polygone, in regards to point number 2 that Josh points out, the issue is that when you use the syntax:
enigma::tile t = dit->second.tiles[i]; t.alpha = 1;
tile is not a pointer to a struct, but a direct struct. Therefore, the first line copies the original structure. In the second line, the copy's alpha field is assigned 1, and since the copy is not used after that, it basically has no effect. What you want to do instead is to either use a pointer instead, like so:
enigma::tile* t = &(dit->second.tiles[i]); t->alpha = 1;
or use a reference variable, which in this case basically the same as using a pointer but with more convenient syntax.
enigma::tile& t = dit->second.tiles[i]; t.alpha = 1;
. Both of the above solutions ensures that you are modifying the original instead of a copy that is not used later.
|
|
|
Logged
|
|
|
|
polygone
|
|
Reply #3 Posted on: June 15, 2013, 04:17:33 pm |
|
|
Location: England Joined: Mar 2009
Posts: 794
|
I'm not in a programming mood at all now, I didn't even read the code that Josh put I just wondered why he referenced alpha in particular... I never tested any of the tile manipulation functions when I wrote them so likely some of them aren't correct.
|
|
|
Logged
|
I honestly don't know wtf I'm talking about but hopefully I can muddle my way through.
|
|
|
Josh @ Dreamland
|
|
Reply #4 Posted on: June 15, 2013, 05:20:00 pm |
|
|
Prince of all Goldfish
Location: Pittsburgh, PA, USA Joined: Feb 2008
Posts: 2950
|
Don't worry, forthevin; I've already corrected this and will commit it with countless other changes, soon.
For future reference of those who are concerned, [snip]const struct &x = y[/snip] to copy big structs by reference. Lose the const to allow assigning to its members.
|
|
|
Logged
|
"That is the single most cryptic piece of code I have ever seen." -Master PobbleWobble "I disapprove of what you say, but I will defend to the death your right to say it." -Evelyn Beatrice Hall, Friends of Voltaire
|
|
|
Goombert
|
|
Reply #5 Posted on: June 15, 2013, 08:05:38 pm |
|
|
Location: Cappuccino, CA Joined: Jan 2013
Posts: 2993
|
Josh, you still have not answered me, why are you working on the engine all of the sudden?
|
|
|
Logged
|
I think it was Leonardo da Vinci who once said something along the lines of "If you build the robots, they will make games." or something to that effect.
|
|
|
Josh @ Dreamland
|
|
Reply #6 Posted on: June 15, 2013, 08:50:10 pm |
|
|
Prince of all Goldfish
Location: Pittsburgh, PA, USA Joined: Feb 2008
Posts: 2950
|
Because the engine needs it, too. It isn't just the compiler. ISO 11 is becoming commonplace: the game is set; the cards are on the table. I definitely need to concern myself with the compiler, but that'll only do us so much good if the engine is a wreck.
Make no mistake, the engine is a wreck. It's much better since you added that Bridges folder as I asked. One of the changes in this huge shitball I'm about to commit moves the rest of the X11 code there that shouldn't be in with the rest of the engine. It's amazing what you can find by enabling all compiler warnings.
It's also amazing that no one else fucking enables compiler warnings, or reads the compile output in ENIGMA—half the warnings I've fixed are checkable with the default C::B build settings. Not that anyone maintains the CBP. I committed a huge fix in that not long ago, and have only fixed more of it now.
|
|
|
Logged
|
"That is the single most cryptic piece of code I have ever seen." -Master PobbleWobble "I disapprove of what you say, but I will defend to the death your right to say it." -Evelyn Beatrice Hall, Friends of Voltaire
|
|
|
|
Ideka
|
|
Reply #8 Posted on: June 16, 2013, 11:02:37 am |
|
|
Joined: Apr 2011
Posts: 85
|
What about -Wextra? I always use it along with -Wall and -O2, though I really don't know how useful it is . Also I'm pretty sure gcc doesn't catch certain things at -O0 so you should probably have -O3 in regular run (or maybe -O2?).
|
|
|
Logged
|
|
|
|
|
Josh @ Dreamland
|
|
Reply #10 Posted on: June 16, 2013, 12:31:07 pm |
|
|
Prince of all Goldfish
Location: Pittsburgh, PA, USA Joined: Feb 2008
Posts: 2950
|
GCC has fifty warnings flags that need enabled (and a few that need disabled, because I fell they -should- be standard by now, and will be soon enough). One of ENIGMA's biggest offenses is use of variable-length arrays. It's a GCC extension that's too convenient to ignore.
I'm hung up on polyfuck's data structure code. It's a typecast nightmare. Total mess.
|
|
|
Logged
|
"That is the single most cryptic piece of code I have ever seen." -Master PobbleWobble "I disapprove of what you say, but I will defend to the death your right to say it." -Evelyn Beatrice Hall, Friends of Voltaire
|
|
|
polygone
|
|
Reply #11 Posted on: June 16, 2013, 02:39:01 pm |
|
|
Location: England Joined: Mar 2009
Posts: 794
|
GCC has fifty warnings flags that need enabled (and a few that need disabled, because I fell they -should- be standard by now, and will be soon enough). One of ENIGMA's biggest offenses is use of variable-length arrays. It's a GCC extension that's too convenient to ignore.
I'm hung up on polyfuck's data structure code. It's a typecast nightmare. Total mess.
Isn't there a thing called find and replace that you could maybe use?
|
|
|
Logged
|
I honestly don't know wtf I'm talking about but hopefully I can muddle my way through.
|
|
|
Josh @ Dreamland
|
|
Reply #12 Posted on: June 16, 2013, 02:47:34 pm |
|
|
Prince of all Goldfish
Location: Pittsburgh, PA, USA Joined: Feb 2008
Posts: 2950
|
I have done my best to use it where possible. I'm trying to avoid breaking the code in the process of removing those warnings, and fixing the disastrous way in which the code treats [snip]typename t[/snip] and [snip]variant[/snip] as interchangeable types.
I've completed the redecoration, at any rate. I have about 150 more warnings to fix, and then I'll commit it, and we can attempt some regression testing.
|
|
|
Logged
|
"That is the single most cryptic piece of code I have ever seen." -Master PobbleWobble "I disapprove of what you say, but I will defend to the death your right to say it." -Evelyn Beatrice Hall, Friends of Voltaire
|
|
|
|