isObject giving false positives

#1 function isObject(%value) returns 'true' if %value is not '0' or ''

#2 As the first echo shows, that doesn't necessarily seem to be the case. Also, typing this in the console worked:

==>"7.98737 12.3106 1.05857".getClassName();
DefaultParticle

So it seems there's something crazy going on with object resolution in general.

EDIT: Oh. Just found this thread with the exact same problem. Also, the console log above is slightly incorrect - I was working from memory. It actually did give the class as ParticleData, not DefaultParticle :P.

#3 isObject() is not to be used to validate input and only meant to be used to ensure that an object handle is still valid before attempting to use it. Meaning you should only ever use isObject() on variables you know for a fact should only be an object handle. It is your responsibility to validate input. In your case you should be doing:

function AIMoveToAction::onStart(%this, %obj, %data, %resume)
{
   %count = getWordCount(%data);
   if(%count == 3)
      %obj.setMoveDestination(%data);
   else if(!%count && isObject(%data))
      %obj.setMoveDestination(%data.getPosition());
   else
      error("AIMoveToAction::onStart(): %data is invalid! \""@ %data @"\"");
}

#4 Okay, that's fair enough - thanks for the clarification. I'm still worried, though. That second example could introduce a major bug if it slipped into something insidiously. Especially since the function which seems designed to catch such errors, isObject, does not function how you'd expect it to.

#5 Yeah, way down in SimIdDictionary::find() that first argument is being converted to a U32 and apparently there is something with that id number actually in existence.

Perhaps that should be modified to check the number of arguments and emit a warning in debug builds.

#6 I think that DefaultParticle(7) means the object has ID 7, which is the part before the decimal place in the vector. Which makes some small amount of sense, but I still don't find it reasonable in any way.

A warning in debug fields sounds like a nice non-intrusive way to solve this, but to be frank, I'd advocate for an actual fix in the name resolution algorithm. Eliminating false positives shouldn't hurt anybody's existing scripts, and it will provide more robust ways to use the language (for example, being able to use isObject on arbitrary data).

This will sound a bit ranty, but it's stuff like this that makes me want to stay out of TorqueScript as much as humanly possible. I guess I've been running up against it a lot lately, since I've been doing a lot more TorqueScripting instead of my usual diet of C++ and other programming languages. I'm getting hung up on minor things, but it's frustrating when nobody else really thinks it's a problem ;P.

EDIT: Little fix to Sim::findObject

simManager.cpp @354
   if(c >= '0' && c <= '9')
   {
      // it's an id group
      const char* temp = name + 1;
      for(;;)
      {
         c = *temp++;
         // Problem:
         if(!c)
            return findObject(dAtoi(name));
         else if(c == '/')
         {
            obj = findObject(dAtoi(name));
            if(!obj)
               return NULL;
            return obj->findObject(temp);
         }
         // Fix:
         else if (c < '0' || c > '9')
            return NULL;
      }
   }

#7 I agree with you, but "at what cost" comes to mind. I prefer non-intrusive even if it costs me more in discipline (to a point - it can get ridiculous, of course). I feel your pain, though - I spend the vast majority of my day in Torquescript.

But by passing in an integer value that is the SimID of an object you're really limiting yourself to what you can do to validate that (especially since SimObjects don't require a name in addition to the ID). If you're looking for objects by name you should almost never see a false positive, but all SimObjects have an integer ID value - testing any number would have some chance of coming back positive. See simDictionary.cc line 265 - sadly, it's pretty simplistic.

On the up-side, it looks relatively quick all things considered.

#8 Well, I reckon this is one of those cases where there is no cost. My change simply ensures that things like "7.2" don't even get a chance to become an object. Clearly this is not an object handle, it's a float. Without the check I added, strtol would get called on "7.2", which would return the 7. Passing a valid integer such as "0126", or a string that doesn't start with a digit (i.e., a valid object name, not a handle) isn't affected.

I guess I'm coming from the land of modern scripting/interpreted languages, where you worry more about developer productivity and ease-of-use. Every hour you don't spend wondering why your vector is turning into a particle datablock is an hour you can be actually producing code, so it's important that things work exactly as advertised.

With T3D moving open-source and hopefully attracting tons of new users, I think it's more important than ever to fix issues like this. It's not good enough to accept that this is the way the language works and has always worked; we need to actually evaluate everything from the perspective of someone new to programming. What would they expect to happen?

Sorry - this really gets my goat for some reason. Imma go eat some scones...

#9

if(c >= '0' && c <= '9')
   {
      // it's an id group
      const char* temp = name + 1;
      for(;;)
      {
         c = *temp++;
         if(!c)
            return findObject(dAtoi(name));
         else if(c == '/')
         {
            obj = findObject(dAtoi(name));
            if(!obj)
               return NULL;
            return obj->findObject(temp);
         }
      }
   }
   else
      return NULL; // should accomplish the same thing?

I haven't tested it - but it looks like it would do the same thing without the extra test.

#10 It's not an issue ... it's a characteristic ...

[/couldn't find the meme I wanted :( ]

drools scones

#11 There is a rather nasty hack in the scripting interpreter which ignores object names with spaces when setting objects. isObject() doesn't contain the same logic and directly uses Sim::findObject, so it's no wonder there is an inconsistency.

Ideally in this case Sim::findObject should be fixed.

#12 Richard: check the rest of the function. If the identifier doesn't start with a digit, it is treated as a name. My change just checks that if the first character is a digit (and therefore the string is not a valid name), the rest of the characters are too. That's why the comparison needs to happen inside the for loop.

Steve: these characteristics are exactly what I'm on about. EDIT: is this the one you were looking for? :P

James: thanks for the tip. What do you mean by 'ignored'? As in, names with spaces are valid?

#13 I think that you might be chasing your tail a bit here ...

#14 This is just a bug in your TS code. Write over the end of an array in C++ and watch all the magical things that happen. :)

#15 lol - figured I might be overlooking something; slap-dash glance while waiting for a compile and all....

Ok, "officially" we treat object names with spaces as invalid. I agree that Sim::findObject() should be fixed. And perhaps that hack in the interpreter should be made more elegant while we're at it - oh, wait, the whole interpreter is a hack.... lol

It is fun, though.

#16 Tim: not sure how serious you are, so I'll take that in the spirit I hope it was intended! You're right, I shouldn't have upheld C++ as an example of a pleasant-to-work with language (it's not!)... at least it's statically typed. Heck, any sort of typing is nice!

Pull-requested.

#17 Heck, all kinds of typing is involved - just look at all of these posts....

#18 In this case, I would agree with Daniel's idea for the check for it being a valid integer or name.
That's the easiest solution that doesn't restrict regular usage, while also preventing some odd behavior. It's a pretty good middle-ground solution I think.

#19 Having checked my own script I've got to say Nathan's version is the way to do it. I send vector/positions or IDs and check for word length first on my receiving function to determine what has arrived.

#20 Steve: the problem is, there's a more general issue with name resolution in Sim::findObject that was allowing statements like "7.125".getClassName(); If that's not a bug, I don't know what is. Plus, being able to use isObject is more succinct and makes code read more easily. That's the biggest win, IMO.