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

Added transparency for pickups #433

Merged
merged 5 commits into from May 14, 2018

Conversation

Projects
None yet
4 participants
@NFSMONSTR
Copy link
Contributor

NFSMONSTR commented May 1, 2018

It seems code of changing weapon corona color in master written to use additive blending

Also in GTA 3 and VC(at least in VC) using additive blending for weapon coronas(clickable)


So there is a directx code that i got with MS PIX(part of directx sdk):
image
As you can see GTA VC is using additive blending
Also you might notice that in one moment corona look the same as in GTA 3, so we can make a conclusion that in it used almost the same render code:
image

Here is output of PIX for more detail researching

Also I tried to do the same for GTA 3 but all my tries(I tried to use MS PIX and apitrace) were unsuccessful


So I added choosing of blending mode in setBlend and DisplayParameters: default(alpha-blending) and additive blending
But here are some problems:

  1. Almost everywhere used DisplayParameters and setBlend, but in some places instead of they direcrly used glEnable(GL_BLEND) that causing problems with adding multiply blend modes
  2. Also in some places render code expects that alpha-blending enabled
  3. Also we have OpenGLRenderer class so I think is better to place all OpenGL-dependend code into it and add some wrappers to use it in GameRenderer, MapRenderer and other classes instead of using direct OpenGL calls, because if later we will decide to add other graphical backend it will create some additional difficulties(May be should add this to issues as improvement?)

To do:

  • Fix render bugs caused by 1 and 2

Special thanks to @JayFoxRox for help

@@ -75,6 +75,8 @@ class Renderer {
Textures textures;
/// Alpha blending state
bool blend;
/// Blending mode 0 - glBlendFunc(GL_SRC_ALPHA, GL_ONE_MINUS_SRC_ALPHA); 1 - glBlendFunc(GL_ONE, GL_ONE);
size_t blendMode;

This comment has been minimized.

@JayFoxRox

JayFoxRox May 1, 2018

Collaborator

Please create a new enum if we are going to support multiple blending modes. We could even consider removing the bool for blending and using the enum values to disable blending.

This comment has been minimized.

@NFSMONSTR

NFSMONSTR May 1, 2018

Contributor

Good idea. Thank you

case BLEND_ADDITIVE:
glBlendFunc(GL_ONE, GL_ONE);
break;

This comment has been minimized.

@JayFoxRox

JayFoxRox May 2, 2018

Collaborator

I'd rather see

default:
  assert(false);
  break;

We'll have undefined behaviour anyway in that case - so might as well catch it in the debugger

/**
* Enum used to determine which blending mode to use
*/
enum BlendMode {

This comment has been minimized.

@JayFoxRox

JayFoxRox May 2, 2018

Collaborator

We use modern C++. Consider enum class

@JayFoxRox

This comment has been minimized.

Copy link
Collaborator

JayFoxRox commented May 2, 2018

I've only skimmed over the code, and aside from what I've noted, this LGTM.
Thanks for taking the time to use PIX and all - greatly appreciated :)

@NFSMONSTR

This comment has been minimized.

Copy link
Contributor

NFSMONSTR commented May 3, 2018

Thank you for reviewing

@NFSMONSTR

This comment has been minimized.

Copy link
Contributor

NFSMONSTR commented May 3, 2018

I dont sure what to do with code like this:
(GameRenderer.cpp)

void GameRenderer::drawTexture(TextureData* texture, glm::vec4 extents) {
    glUseProgram(ssRectProgram);
    ...
    glEnable(GL_BLEND);
    glUniform2f(ssRectOffset, extents.x, extents.y);
    glUniform2f(ssRectSize, extents.z, extents.w);
    ...

It uses OpenGL calls directly instead of calling Renderer class
Also it just enables GL_BLEND expecting that alpha blending already enabled

So what should I do in this PR?
Should I rewrite it to use Renderer class or just add glBlendFunc(GL_SRC_ALPHA, GL_ONE_MINUS_SRC_ALPHA);?

@ShFil119

This comment has been minimized.

Copy link
Member

ShFil119 commented May 7, 2018

Using rebase you won't have extra commits. ;)

@NFSMONSTR

This comment has been minimized.

Copy link
Contributor

NFSMONSTR commented May 7, 2018

Should I squash these commits?

@ShFil119

This comment has been minimized.

Copy link
Member

ShFil119 commented May 7, 2018

I would do:

  • drop merge commit using interactive rebase,
    EDIT I haven't been using merge for long time. @JayFoxRox pointed there's possibility that interactive rebase doesn't include merge commit. You can use git reset --hard or interactive rebase with flag --preserve-merges (as far I remember).
  • git remote add up https://github.com/rwengine/openrw
  • git fetch up
  • git rebase up/master
  • check is everything alright and push. ;)

Please visit IRC for discussion. ;)

@NFSMONSTR

This comment has been minimized.

Copy link
Contributor

NFSMONSTR commented May 9, 2018

Thank you. I'll try to do that

@NFSMONSTR NFSMONSTR force-pushed the NFSMONSTR:add_transparent_pickups2 branch from 0e85aea to 45ee3ad May 9, 2018

@NFSMONSTR NFSMONSTR changed the title [WIP]Added transparency for pickups Added transparency for pickups May 10, 2018

@danhedron danhedron merged commit a8cafe7 into rwengine:master May 14, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment