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

Frag message output with icons #11

Merged
merged 1 commit into from Nov 24, 2016

Conversation

@ldrone
Copy link
Contributor

commented Nov 20, 2016

No description provided.

@ldrone ldrone force-pushed the ldrone:frag_msg branch from 62ad5d6 to 76d1d21 Nov 20, 2016
@The-Gig

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2016

Just adding a link to the previous discussion on the topic:
#8

@The-Gig

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2016

Excuse me Idrone, you did not answer to a question I did here:
#8 (comment)

PS: You already know, I don't see any reason for hiding the attacker name, in case of indirect kill.

PPS: just asking for curiosity... why does the "case MOD_BFG:" section is completely empty? Because so it automatically takes the same settings as "case MOD_BFG_SPLASH" which is right under it? You know I'm not good with C...

UPDATE: Wait, for some reason I was not seeing your latest changes. I have to check them now.

Update2: About the "we don't know what is was", I see you added some code there (depending from obituary mode), but I don't exactly understood what it does (I would have expected something like "causeShader = cgs.media.skullShader").
Update3: Maybe it inherits it from this section above? I have no idea...

+      default:
+              message = NULL;
+              causeShader = cgs.media.skullShader;
+              break;

About drawing attacker name (if available), I see now you show it. Thank you!

@The-Gig

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2016

Another thing: when someone is killed by a teammate, shouldn't the iconographic version mention this, too? Like
x [skull icon] y (TEAMMATE)

@ldrone ldrone force-pushed the ldrone:frag_msg branch 3 times, most recently from 7a0fe49 to 06ae05d Nov 21, 2016
@ldrone

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2016

PPS: just asking for curiosity... why does the "case MOD_BFG:" section is completely empty? Because so it automatically takes the same settings as "case MOD_BFG_SPLASH" which is right under it? You know I'm not good with C...

It "falls through" to the next case since there is no break; statement, which would exit the switch and resume after it. So the compiler continues to read lines until the next break; is encountered, then it jumps out of switch.

@ldrone ldrone force-pushed the ldrone:frag_msg branch from 06ae05d to 8812264 Nov 21, 2016
@The-Gig

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2016

About the bfg question... so it's like I guessed. Thank you!

@sago007

This comment has been minimized.

Copy link
Member

commented Nov 21, 2016

Ok, I have checked it out and also tryed playing it. Looks good. A bit hard to read on my screen but I could not read the console either, so it does not matter.

Regarding the code:
Your code is indented with spaces while most of the code is indented with tabs. I must admit that I have done the same at some point. Github just makes it much more obvious. I am not taking side in the spaces vs tab argumentation but it should be the same in all of the code.

Also something like this:

 Q_strncpyz(lastFrag->targetName, targetName, 32);

The "32" should either be replaced by a "#define MAX_FRAG_NAME_LENGTH" or a "sizeof".

static int fadeTime = 7000;

should properly be

const int fadeTime = 7000;

I would greatly prefer curly brackets here:

if (cgs.fragMsg[i].teamFrag)
            CG_DrawStringExt(x + x_offset, y + (i * TINYCHAR_HEIGHT),
                  "(^1TEAMMATE^7)", hcolor, qfalse, qfalse,
                  TINYCHAR_WIDTH, TINYCHAR_HEIGHT, 0);

While it is only 1 logical line it is 3 physical lines and I would mark it. You also used curly brackets other places where the if content was more than 1 physical line.

Also

            if (ent->generic1) lastFrag->teamFrag = qtrue;
            else lastFrag->teamFrag = qfalse;

could be

            if (ent->generic1) {
                lastFrag->teamFrag = qtrue;
            }
            else {
                lastFrag->teamFrag = qfalse;
            }

or maybe just

lastFrag->teamFrag = ent->generic1;

Aside from that the code looked solid.

Perhaps the commits should be squashed before merging. While it is easy enough to read all the commits as one in this pull request it might be harder in the commit history.

@ldrone ldrone force-pushed the ldrone:frag_msg branch from 8812264 to 91f5ca7 Nov 22, 2016
@ldrone

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2016

I changed the cg_kickScale to show 1.0 as Gig suggested in #9. Should I leave that here since it's a minor change or else how would I go to make that a separate commit?

@sago007

This comment has been minimized.

Copy link
Member

commented Nov 22, 2016

Doesn't matter. Just let it be in this.

@ldrone ldrone force-pushed the ldrone:frag_msg branch 3 times, most recently from 4fc4bd7 to a2d1478 Nov 24, 2016
@ldrone ldrone force-pushed the ldrone:frag_msg branch from a2d1478 to 147dca4 Nov 24, 2016
@ldrone

This comment has been minimized.

Copy link
Contributor Author

commented Nov 24, 2016

Replaced spaces with tabs. Currently in my editor, it's hard to see how the tabs between words will look on github.

@sago007

This comment has been minimized.

Copy link
Member

commented Nov 24, 2016

Looks good to me!

@sago007 sago007 merged commit fbd7abf into OpenArena:master Nov 24, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ldrone ldrone deleted the ldrone:frag_msg branch Nov 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.