Float, Boolean, SAME THING

nullptr

Had an issue with some 'optimizations' I checked in a few days back having to do with switching the state of an entity and precaching resources. The original component didn't allow for that, and the rest of the code had been structured around that, so it took a bit to get everything straightened out.

Yesterday a new issue popped up in Jira about one of our tools crashing during a specific operation. I took it since I assumed it was more trouble from the optimizations I did. Turns out it was related to a different feature that had just been added in.

Here's the line of code I crashed on:

    m_pEntity->GetControllerByCategory("Physics")->Deactivate();

Well that looks simple, for some reason the query is returning a NULL. Due to the fact that it's happening at a point that the entity is being reconstructed, this makes sense. So a simple wrapping of this in a sanity check should fix it.

Wait, what's this?

    m_pEntity->GetControllerByCategory("Physics")->Activate();

But... wait... that's like 4 lines later if we ignore white space, and all we did between these two was set a boolean value that the Physics has no idea about? The hell?

Followed immediately by:

    float tempForward = m_bForward;



    while (time > 0)

    {

        if (m_bForward != tempForward)

        {

            m_bOriginalForward = tempForward;

        }



        m_pEntity->SetPosition(m_pEntity->GetPosition());



        // Messy block of code that has a lot of duplication within conditional tests goes here.

    }

THE STUPID IT BURNS

How did this pass peer review?

Evilweasil

@CodeNinja said:

How did this pass peer review?

You're making the assumption that it was reviewed properly.

This is a fallacy.

nullptr

@Evilweasil said:

@CodeNinja said:
How did this pass peer review?

You're making the assumption that it was reviewed properly.

This is a fallacy.

It took me an hour and a half to build up the courage to open CodeCollab to see who was on the review. I had a sick feeling it was me.

Thankful it wasn't!

I'm going to fix the critical, 'OMG it's crashing!' bug so they stop re-opening my unrelated issue, then write up about 6 new issues.

Might get out before I hit 12 hours today. That'd be nice.

Ben L.

Why is it that whenever I see hungarian notation and the word entity I immediately think of Source Engine and can't think of anything else?

I need more sleep.

MathNerdCNU1

@CodeNinja said:

m_pEntity->SetPosition(m_pEntity->GetPosition());

Am I the only one that sees that as a WTF? Or are there some hidden concurrency WTFs lurking deeper?

Ben L.

@MathNerdCNU said:

@CodeNinja said:

m_pEntity->SetPosition(m_pEntity->GetPosition());
Am I the only one that sees that as a WTF? Or are there some hidden concurrency WTFs lurking deeper?

Probably some extra ~~logic~~ magic in the setter...

@MathNerdCNU said:

Am I the only one that sees that as a WTF? Or are there some hidden concurrency WTFs lurking deeper?

I was going to post about how fascinated I was about this line, even though I assume it might be written for intended side-effects in the setter (like Ben L. said).

Mcoder

@Zecc said:

@MathNerdCNU said:
Am I the only one that sees that as a WTF? Or are there some hidden concurrency WTFs lurking deeper?
I was going to post about how fascinated I was about this line, even though I assume it might be written for intended side-effects in the setter (like Ben L. said).

Yep. Even tough it is probably there for a reason (that emans, it does something), the fact that there is a reason to write a line like that is a WTF in itself.

powerlord

@Ben L. said:

Why is it that whenever I see hungarian notation and the word entity I immediately think of Source Engine and can't think of anything else?
I need more sleep.

Silly Ben, you should be issuing edicts, not messing around with entities!

RichP

@CodeNinja said:

if (m_bForward != tempForward)
     {
          m_bOriginalForward = tempForward;
     }

Correct code would be:
if (m_bForward == tempForward)
   {
      // Do nothing.
   }
else if (m_bForward != tempForward)
   {
      m_bOriginalForward = tempForward;
else
   {
      printf("omfg haxor alert"");
    }

nullptr

@MathNerdCNU said:

@CodeNinja said:

m_pEntity->SetPosition(m_pEntity->GetPosition());
Am I the only one that sees that as a WTF? Or are there some hidden concurrency WTFs lurking deeper?

I was wondering if anyone would catch that.

Needless to say that line didn't survive, since it didn't actually do anything.

There was even more fun on the tool side. Like when he tried to sort a ListView by passing it to a sorter function that took a TreeView. Oddly enough the compiler would let you do it without throwing a warning, it just blew up in a random memory location when it tried to run that function. That was fun to track down, as the call stack got totally corrupted by the crash.