Not logged inOpenClonk Forum
Up Topic Colored lights

This board is threaded (i.e. has a tree structure). Please use the Reply button of the specific post you are referring to, not just any random button. If you want to reply to the topic in general, use the Post button near the top and bottom of the page.

Post Reply
In Response to Marky
A few thoughts about the patch:

>* LIGHT_DEBUG_COLOR seems to work very differently from LIGHT_DEBUG. A comment explaining the difference would be nice.


Maybe I'll remove it entirely. Done

>* You often put two spaces between type and name in variable declarations. Is that another style thing people have come up with?


No, I'll check where I did that. My intention is using one space onyl.

>* That "lightColorCoord" definition looks really funky. How about just "lightCoord.st - vec2(0.0,0.5)"?
>* What is the "pink shade for debugging" about? Should that be LIGHT_DEBUG_COLOR?


I'll check it. Done

>* Could use better names than "C4DP_First" and "C4DP_Second". But that's probably something I should change afterwards.


Ok.

>* Also huh, the "shadow" naming really doesn't make sense, does it.


Where was that?

>* You are declaring "alpha" in "DrawVertex" so it scopes over multiple "case" alternatives. That is a warning for multiple C++ compilers.


Do you have a suggestion how to improve it?Done

>* SetColour: We have "GetRedValue" & co for extracting bytes from a RGB word


Cool, I'll implement that then.Done

>* The change to C4FoWLight::Update only consists of changing the curly brackets to a different style? You have lots of these. If you feel strongly about the code style, I would suggest making it a separate commit.


We have a code style document for C4Script somewhere. Usually I apply that also to the engine code when I see something. It's a habit from my work place. Creating a separate commit seems like a lot of effort for this patch. I'll proceed as you said in future commits.

Done.

>* A few commented-out "GL_SCISSOR_TEST" in "C4FoWRegion::BindFramebuf".


Thanks, I'll check those.Done.

>Please don't use merge - instead squash & rebase and write a good overview commit message for the whole thing.


I am not too familiar with the git workflow. I am  comfortable with squashing and rebasing on my own branch, but I want to keep the single commits somewhere. So... I cherry pick my commits to the master branch, squash them into one commit with good description and then push that?

Powered by mwForum 2.29.7 © 1999-2015 Markus Wichitill