Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed alpha issues #969

Merged
merged 7 commits into from Jul 2, 2016

Conversation

Projects
None yet
3 participants
@faissaloo
Copy link
Contributor

commented May 30, 2016

Fixes #967 and #956 .
Fyi I had to #include "Graphics_Systems/General/GScolors.h" so I could use draw_get_alpha() in GSbackground.

@faissaloo faissaloo changed the title Fixed #967 and #956 Fixed alpha issues May 30, 2016

@RobertBColton

This comment has been minimized.

Copy link
Member

commented May 30, 2016

Hold up I found something that @polygoned did that caused all of this.

alpha=bind_alpha(alpha)*draw_get_alpha();

Should just be:

alpha=bind_alpha(alpha);

This goes back to another issue, please see my comments on #967 the real fix is this:

// this one does not take color and alpha in GM so the extra parameters in ENIGMA should default
// to the set color and alpha
void draw_sprite(int spr, int subimg, gs_scalar x, gs_scalar y, int color = draw_get_color(), gs_scalar alpha = draw_get_alpha());
// functions that do take color and alpha in GM should simply leave the default arguments to c_white
// and 1.0 for the alpha, draw_sprite_ext is one of those functions
void draw_sprite_ext(int spr, int subimg, gs_scalar x, gs_scalar y, gs_scalar xscale = 1, gs_scalar yscale = 1, double rot = 0, int color = 16777215, gs_scalar alpha = 1);

If you want I can dig through the manual and pull out a list of all functions that should take color and alpha for you. But once you make those changes I'll gladly merge this and we can close both of those issues.

@faissaloo

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2016

No, what @polygoned did with regards to the alpha was correct because alpha has a combining effect in GM, with your fix if you were to specify an alpha it would override the one set by draw_set_alpha().
As for color, the color set by draw_set_color() only affects "fonts, forms, primitives and 3D" and so using it as the default for anything else would cause incompatibility, the default should be c_white for those non-affected functions.

@RobertBColton

This comment has been minimized.

Copy link
Member

commented May 31, 2016

I am a little bit confused so I went back and tested again, but couldn't figure this out. Which drawing functions have a combining effect?

For example, when I do the following in GMS, the set alpha does not seem to be multiplied to the passed alpha in the last call and the sprite appears fully opaque:

draw_set_alpha(1);
// fully opaque
draw_sprite(sprite0, 0, 0, 0);
draw_set_alpha(0.5);
// half opaque
draw_sprite(sprite0, 0, 0, 30);
// fully opaque
draw_sprite_ext(sprite0, 0, 0, 60, 1, 1, 0, c_white, 1);
@faissaloo

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2016

You're right, turns out I must've imagined it ¯_(ツ)_/¯ I'll go fix that now.

@faissaloo

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2016

I'll do the other stuff now

@faissaloo

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2016

Btw, the draw_sprite* and draw_background* functions don't use draw_set_color(), they use c_white, which I just tested. They do use draw_set_alpha() though.

@RobertBColton

This comment has been minimized.

Copy link
Member

commented May 31, 2016

You are right, I went back and tested that code again with draw_set_color(c_yellow); at the top and none of them were affected. So for sprites and backgrounds the default color should be c_white, and the default alpha should be draw_get_alpha. For consistency let's do the same for draw_sprite_ext where we make the arguments optional. Then in each of the functions we can use bind_alpha, without multiplying it, to normalize the alpha between 0 and 1.

I noticed @polygoned never touched fonts, so I think we should just leave them alone. But if we did override the font functions to make color and alpha an option for all of them, I would set the default color to draw_get_color and the alpha to draw_get_alpha.

So to summarize:

  • All drawing functions that are supplied an alpha will call bind_alpha to normalize the alpha between 0 and 1.
  • Drawing functions that do not take an alpha will be normalized by extension of using draw_get_alpha which is normalized or being fully opaque.
  • Sprites and backgrounds use c_white as the default color unless the user supplies one.
  • Sprites and backgrounds use draw_get_alpha unless the user supplies an alpha.
  • Fonts will use draw_get_color and draw_get_alpha if the user supplies only one or neither.
  • Shape functions such as circle/line/rectangle will behave the same as fonts.

Then our API will be compatible with GMS and legacy GM as well as be consistent. Thoughts? Let me know and I'm going to go test the other drawing functions real fast.

Text:

draw_set_color(c_yellow);
draw_set_alpha(0);
// visible
draw_text_color(0, 0, "hello", c_red, c_yellow, c_blue, c_green, 1);
// not visible
draw_text(0, 30, "hello");

Shapes:

draw_set_color(c_yellow);
// yellow
draw_circle(0, 0, 50, false);
draw_set_alpha(0);
// yellow and not visible
draw_circle(0, 150, 50, false);
// not visible
draw_circle_color(0, 300, 50, c_red, c_red, false);
draw_set_alpha(1);
// red and visible
draw_circle_color(0, 450, 50, c_red, c_red, false);
@faissaloo

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2016

You are right, I went back and tested that code again with draw_set_color(c_yellow); at the top and none of them were affected. So for sprites and backgrounds the default color should be c_white, and the default alpha should be draw_get_alpha. For consistency let's do the same for draw_sprite_ext where we make the arguments optional. Then in each of the functions we can use bind_alpha, without multiplying it, to normalize the alpha between 0 and 1.

This is all done.

I noticed @polygoned never touched fonts, so I think we should just leave them alone. But if we did override the font functions to make color and alpha an option for all of them, I would set the default color to draw_get_color and the alpha to draw_get_alpha.

For now, I think we should keep it how it is (without any defaults) but should, after this merge further talk about it, maybe in an issue or something, because I for one am for it because it would make it consistent with sprites and backgrounds, but I would like to know how everyone else feels about it.

Fonts will use draw_get_color and draw_get_alpha if the user supplies only one or neither.
Shape functions such as circle/line/rectangle will behave the same as fonts.

It's important to note that neither of these arguments exist in any of the font functions yet and are missing in some of the shape functions.

@faissaloo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2016

Is this substantial enough to warrant adding myself to the header? It says I should ask here:
https://enigma-dev.org/docs/Wiki/Coding_conventions#Legalese
Also, I just found out that draw_set_color() is also affecting 3D drawing of textures, that's not supposed to happen right? I don't remember it doing that in GMS.

@JoshDreamland

This comment has been minimized.

Copy link
Member

commented Jun 15, 2016

Up to you; the change spans lots of lines, but it's trivial.

@faissaloo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2016

I just tested the 3D stuff, the behaviour is correct, so no need to change it. It should be ready for merge now unless anyone has anything else to say.

@RobertBColton

This comment has been minimized.

Copy link
Member

commented Jun 15, 2016

Sorry I've been neglecting this because of school work. I wanted to pull it locally first and test it, I'll make sure to do that tonight. I just want to double check some drawing edge cases. I assume you've tested various games to make sure they still render properly?

Edit: Ok I pulled your branch and I see it's up to date with ENIGMA's master. But I now notice the problem everyone else has been having with ENIGMA freezing when you go to compile. If I go to ENIGMA settings and switch it to Direct3D9 it builds, then if I switch it back to OpenGL1.1 it builds. The FPS example runs fine. I don't think this was you but it was some other PR we did since the last portable that caused this.

@faissaloo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2016

I suspected you'd be concentrating on school around about now lol, I've tested the basic functions and I've been actively using it as my main copy to work on some games and I've not noticed any issues so far, you'd do best to test it yourself just in case though.

@RobertBColton

This comment has been minimized.

Copy link
Member

commented Jun 15, 2016

Anyway, the circle and text drawing examples I posted earlier in this issue worked on master. However, the sprite drawing example did not work, but it does work on faissaloo's branch. So just let me run a few more tests and check out some games.

@faissaloo Also the 3D and 2D primitive and model functions should work just like GMS. What I did was basically the same thing I believe they did. All drawing functions are implemented with those primitive drawing functions as I said which are affected by color and alpha if you do not set one. All of the functions in turn actually call the d3d_model drawing functions (https://github.com/enigma-dev/enigma-dev/blob/bee8534aa22d2bd3804d982e9d223d418ccd0900/ENIGMAsystem/SHELL/Graphics_Systems/Direct3D9/DX9primitives.cpp), except in GL1.1, since it does efficient batching. The only real rendering code we have is all in the model drawing functions.
http://enigma-dev.org/forums/index.php?topic=1375

I should mention too that we do have to overload all of the primitive functions, we can't just do argument defaults. The reason is because it effects the stride of the model, we don't want to throw texture coordinates on there, even if they are defaults, when the user doesn't want them. It makes the model unnecessarily large in memory.

@RobertBColton

This comment has been minimized.

Copy link
Member

commented Jun 16, 2016

I should also mention this is a duplicate of #764 and #956 is a duplicate of #757. I have closed #757 as a duplicate since #956 describes the issue better.

Faissaloo's branch also correctly handles the sprite drawing code I had in #956 just like GMS whereas our current master branch does not. I've saved this in my GMGames folder for future testing as I normally do.

@RobertBColton

This comment has been minimized.

Copy link
Member

commented Jun 16, 2016

@JoshDreamland Everything is looking good, but since that macro is defined all over the place to be the exact same thing, maybe we should put it in the General/GScolors.h file? Like in every graphics systems color.cpp file:

#define bind_alpha(alpha) (alpha>1?255:(alpha<0?0:(unsigned char)(alpha*255)))

This would work if we moved the macro there because we are already including it in order to use draw_get_alpha.

@RobertBColton

This comment has been minimized.

Copy link
Member

commented Jun 16, 2016

My other tests show that this PR fixes both of the original issues. The drawing functions seem to behave much closer to GMS with faissaloo's PR than on the current master. Several games continue to work great including the cubes demo from GMS.

This PR may also fix an issue in someone's game:
http://enigma-dev.org/forums/index.php?topic=1396

It also did fix an issue with the platforming example. It has a fade in and out effect on the background of the room, it works great with Faissaloo's patch here, very buggy on master otherwise. To get it to run on current master or his branch you have to open obj_solid and delete the event for the 'R' button. That's caused by an unrelated issue with object inheritance that I am about to post an issue for.
http://enigma-dev.org/edc/games.php?game=69

@JoshDreamland @faissaloo Now I should mention this other issue.
http://enigma-dev.org/forums/index.php?topic=1651
While this PR does bring us closer to GMS and GM8.1 behavior, the functions now have differences from GM8.1 and lower. This PR brings it closer to GM8.1 by ensuring that image_alpha works correctly. Where GMS and GM8.1 differ from my own tests is that draw_background(ind, x, y); is not effected by draw_set_alpha in GM8.1 but is effected by it in GMS.

In ENIGMA's master branch and GM8.1 the two functions are drawn fully opaque. In GMS and Faissaloo's branch the two functions are drawn half transparent.

draw_set_alpha(0.5);
draw_sprite(sprite0,0,256,0);
draw_background(background0,0,0);

I got GM8.1 working in virtual box and used it to confirm this is the case.
Set Alpha GM8.1

This is why @polygoned requested a setting and I ignored him opting to just keep it the same as GM8.1. So I guess we need to change all those defaults to a macro such as #define DEFAULT_BACKGROUND_ALPHA 1 or #define DEFAULT_BACKGROUND_ALPHA draw_get_alpha()

@faissaloo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2016

@RobertBColton

maybe we should put it in the General/GScolors.h file?

It would conflict with the bind_alpha in GLcolors.cpp, I could always just rename the version of bind_alpha() I'm using to bind_alpha_0_to_1() or something though.

So I guess we need to change all those defaults to a macro such as #define DEFAULT_BACKGROUND_ALPHA 1 or #define DEFAULT_BACKGROUND_ALPHA draw_get_alpha()

Should I put the macros in the .h files themselves or put them in separate .h files like @JoshDreamland is doing here?

@RobertBColton

This comment has been minimized.

Copy link
Member

commented Jun 16, 2016

I'd wait until tomorrow and see what he says because I have no opinion on that. But what we will probably do is add either a checkbox or a text field to the ENIGMA settings frame and I'll show you how to do that.

@JoshDreamland

This comment has been minimized.

Copy link
Member

commented Jun 30, 2016

Only recently decided to just drop my GitHub backlog, because it was too large and noisy for any human to process. Appologies.

Anyway, those info files you linked are to aid the build system in figuring out what's going on without parsing the entirety of each system. Virtually nothing belongs in them; I'm not even sure if they're still used.

That said, these constants are sort of esoteric. I'm not sure we want to expose them to users, so I'd put them in a header that's only included from source files in that graphics system. I don't know that such a header exists, so you may need to add one.

@faissaloo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2016

We'd have to expose them to users in some way or having the macros in the first place is nigh-pointless because it would be too obscure for alot of users to bother with.

@JoshDreamland

This comment has been minimized.

Copy link
Member

commented Jun 30, 2016

I'm missing some context, here. Let me just leave some comments inline.

#include "Graphics_Systems/General/GSprimitives.h"
#include "Graphics_Systems/General/GSbackground.h"
#include "Graphics_Systems/General/GStextures.h"
#include "Universal_System/nlpo2.h"
#include "Universal_System/backgroundstruct.h"
#include "Universal_System/spritestruct.h"

//Note that this clamps between 0 and 1, not 0 and 255
#define bind_alpha(alpha) (alpha <= 0 ? 0: alpha >= 1? 1: alpha)

This comment has been minimized.

Copy link
@JoshDreamland

JoshDreamland Jun 30, 2016

Member

Nit: In graphics programming, bind has a very special meaning; I'd use clamp.

This comment has been minimized.

Copy link
@faissaloo

faissaloo Jun 30, 2016

Author Contributor

On it.

This comment has been minimized.

Copy link
@faissaloo

faissaloo Jun 30, 2016

Author Contributor

Done.

void draw_background_tiled(int back, gs_scalar x, gs_scalar y, int color = 16777215, gs_scalar alpha = 1);
void draw_background_tiled_area(int back,gs_scalar x, gs_scalar y,gs_scalar x1, gs_scalar y1,float x2,float y2, int color = 16777215, gs_scalar alpha = 1);
void draw_background_ext(int back, gs_scalar x, gs_scalar y, gs_scalar xscale = 1, gs_scalar yscale = 1, double rot = 0, int color = 16777215, gs_scalar alpha = 1);
void draw_background(int back, gs_scalar x, gs_scalar y, int color = c_white, gs_scalar alpha = draw_get_alpha());

This comment has been minimized.

Copy link
@JoshDreamland

JoshDreamland Jun 30, 2016

Member

This behavior is... disagreeable, but so is the GM6 behavior. Why are we defaulting the color but not the alpha? This is a confusing concept for people who haven't drank the Yoyo kool-aid. For that reason, I prefer the GM6 behavior. Either way, do we have compatibility macros? It would be a good idea to #define DEFAULT_IMAGE_ALPHA to either of draw_get_alpha() or 1 atop this file, conditioned on those macros. #undef it at the bottom, too.

This comment has been minimized.

Copy link
@faissaloo

faissaloo Jun 30, 2016

Author Contributor

Why are we defaulting the color but not the alpha?

Because that's how it is in GMS, I don't think it's too unreasonable, the color is more accurately defined as 'blend color' rather than 'draw color' when you're dealing with images.

Either way, do we have compatibility macros? It would be a good idea to #define DEFAULT_IMAGE_ALPHA to either of draw_get_alpha() or 1 atop this file, conditioned on those macros. #undef it at the bottom, too.

Aside from that, how do you do compatibility macros? Are they just #ifndef #define #endif definitions that get overridden at compile if needed?

This comment has been minimized.

Copy link
@JoshDreamland

JoshDreamland Jun 30, 2016

Member

I don't think it's too unreasonable, the color is more accurately defined as 'blend color' rather than 'draw color' when you're dealing with images.

In graphics programming, you either input a raw color to a vertex, or you don't. When alpha blending is in play, it's usually grouped with color blending. I think users will be weirded out to discover that we make some distinction, though maybe that seems natural in the Game Maker workflow.

Are they just #ifndef #define #endif definitions that get overridden at compile if needed?

That's correct. I know sorlok added some GM5 compatibility macros, but I don't know if there are any for GM6/7/8.

@JoshDreamland

This comment has been minimized.

Copy link
Member

commented Jun 30, 2016

Yeah, looking this over, I see what Robert was talking about. I would not expose those macros to users; I'd just configure them based on compatibility constants. My objection to exposing them to users is that they are not constants; they're macros that may expand to a function call. Moreover, they're a product of the configuration, which the user should be aware of.

So my suggested pattern for them is as follows:

#if GM_COMPATIBILITY_VERSION < 8100
#  define DEFAULT_ALPHA draw_get_alpha()
#else
#  define DEAFAULT_ALPHA 1
#endif

void draw_whatever(gs_scalar x, gs_scalar y /* ... */, float alpha = DEFAULT_ALPHA);
// ... More such functions ...

#undef DEFAULT_ALPHA

Note that I completely made up that compatibility constant. One may or may not exist.

@faissaloo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2016

My objection to exposing them to users is that they are not constants; they're macros that may expand to a function call. Moreover, they're a product of the configuration, which the user should be aware of.

Right, got you.

Note that I completely made up that compatibility constant. One may or may not exist.

So should I add a COMPLIANCE_LVL for Game Maker 8? COMPL_GM8 or something? Also, does anyone have a copy of GM7 to test if the GM8 and GM7 behaviour is the same, so I know if I can just do a || compliance_mode==COMPL_GM7. If we add another compliance level I don't think it would be a good idea to have a batch one for both GM7 and GM8 because we might discover some other subtle difference between the two later on.

@faissaloo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2016

I've been mulling over this for a bit and I think it's best that we make the compliance modes correspond to integers so we can do comparisons with them, similar to how @JoshDreamland suggested in his example, right now compliance modes are just assigned to numbers in the order that they were added, instead they should go like this so that they can easily be further expanded should we find anymore incompatibilities:

enum COMPLIANCE_LVL {
    COMPL_STANDARD = 0,
    COMPL_GM5 = 5,
    COMPL_GM8 = 8
};

Looking at the changelog for GM81 I don't see any incompatibilities, so I don't think there will be any need for a GM81 COMPLIANCE_LVL so I don't think there will be a need to take subversions into account. This shouldn't break anything because I don't see any code relying on COMPL_GM5 having a specific value.

However, the compliance levels are only used within the compiler, they are never passed to the ENIGMAsystem, so I have no way of knowing what the current compliance level is. My suggestion is that we pull the compliance levels out of settings.h and instead include them in settings.h so we only maintain one copy, then include it wherever we need to use it in the ENIGMAsystem, then at compile the compiler provides the compliance level through complier arguments, -DGM_COMPATIBILITY_VERSION=%i.

@faissaloo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2016

How's this? Someone's going to need to add a GM8 option to the compliance mode menu.
Sorry about some of the trailing tabs being removed, my editor did that, didn't realise until I pushed.

@JoshDreamland

This comment has been minimized.

Copy link
Member

commented Jul 2, 2016

Looks fine; we'll sort out the values for those constants, later.

@JoshDreamland JoshDreamland merged commit 11d7484 into enigma-dev:master Jul 2, 2016

@faissaloo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2016

Either I forgot to commit or it got lost in the force push but this is missing so the compliance mode thing is now always going to use GM8 behaviour (because it compares with <=8 ):

enum COMPLIANCE_LVL {
    COMPL_STANDARD = ~0,
    COMPL_GM5 = 5, //now COMPL_GM567
    COMPL_GM8 = 8
};

Right now it's:

  enum COMPLIANCE_LVL {
    COMPL_STANDARD = 0,    //Standard (enigma) compliance. Default and recommended.
    COMPL_GM567 = 1,       //GM5,6,7 compliance. timeline_running will default to "true". exit will abort single code actions.
    COMPL_GM8 = 2,         //GM8 compliance. exit will abort single code actions.
};

I'll go ahead and fix this later.

@JoshDreamland

This comment has been minimized.

Copy link
Member

commented Aug 9, 2016

There isn’t anything to compare.
master is up to date with all commits from 11d7484.

Seems everything's accounted for, so anything missing would have to have been explicitly reverted. Feel free to send the other commits in a pull request, and we'll get them in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.