BitMask bug in CSceneCollisionManager::getPickedNodeBB()

[warui]

idBitMask testing only checks if at lest one of the bits of node id is set, not all of them. To correct it just change this :

Code: Select all

if (current->isVisible() && 
	(bits==0 || (bits != 0 && (current->getID() & bits))))

to this:

Code: Select all

if (current->isVisible() && 
	(bits==0 || (bits != 0 && ((current->getID() & bits) == bits))))

[niko]

Ah, right, thanks!

[jox]

STOP!

This is wrong, it's not a bug! Actually your fix is a bug!

Sorry, but it's true...

The Parameter is a MASK. It's not an ID. You just have to know how to use it (see below).

The reason for the bit mask is to be able to limit the nodes that possibly get picked. So you give your nodes different types (ids) and reference them via the mask.

Your code is not only a bug but its also wrong!

(Warui, please don't get me wrong, and don't take it personally, this is no offense, I really appreciate your work and effort, but in this case it is like it is. )

Because: Let's say you have several nodes with the following ids (types):

PLAYER -> id=1 (0x0001)
WEAPON -> id=2 (0x0010)
HEALTH -> id=3 (0x0011)

Now if you pick a node with mask 0x0010 (WEAPON) you expect only nodes with the WEAPON id get picked.

But with your code also HEALTH gets picked. Because

Code: Select all

(current->getID() & bits)   == bits
 0x0011             0x0010     0x0010
 HEALTH             idBitMask  idBitMask

equals true!

For what you want to achieve (check all bits at once) you would better use this code:

Code: Select all

if (current->isVisible() && 
   (bits==0 || (bits != 0 && ( current->getID() == bits )))

But then - again - you should rather call it id and not bits or idBitMask. But rather let it be..

And also this concept has one major disadvantage: you're able to pick only one node type at a time!


The solution is as follows:

You just define each type with only 1 (unique) bit set! Like this:

Code: Select all

NT_PLAYER -> id=1  (0x000001)
NT_WEAPON -> id=2  (0x000010)
NT_HEALTH -> id=4  (0x000100)
NT_AND    -> id=8  (0x001000)
NT_SO     -> id=16 (0x010000)
NT_ON     -> id=32 (0x100000)

Now Nikos code works just perfect PLUS you are able to limit picking to several node types at the same time by composing a real bit mask like this for example:

Code: Select all

(NT_PLAYER|NT_WEAPON)

which results in 0x000011.

Now here is where your code becomes a bug! Node Picking will not work anymore with this mask, because there are no nodes with id=0x11 (which your code expects). Only 0x10 or 0x01!

Btw, best way to define the types is by enumerating them. Example:

Code: Select all

enum NODE_TYPES {
  NT_PLAYER =1
  NT_WEAPON =2
  NT_HEALTH =4
  NT_AND    =8
  NT_SO     =16
  NT_ON     =32
};

That's how bit masks work...

Pew, I hope I could make myself clear. See my point warui? (niko?)

regards,
jox

[Tyn]

I thought that myself actually, however there is no reason why it should be a mask instead of an exact match. Why should people be constricted to the power of two counting system caused by this?

It doesn't work as far as I've seen anyway. I had a wall as id=16 and a character as id=1 and the mask set to 16. Which should mean:

character = 0001
wall = 1000

And so the character should be ignored but the collision is still detected on both.

[jox]

I thought that myself actually, however there is no reason why it should be a mask instead of an exact match.

Yes there is one reason, that I also pointed out in my post: you can combine types. That is what I do myself and I wouldn't wanna miss that feature! I have different node types that need to get picked at the same time, but also need to be differentiable via the id. So I used the bit mask as it was supposed to be and it did exactly what I expected.

Why should people be constricted to the power of two counting system caused by this?

Maybe because it makes sense? Maybe because it's common style? And actually why should they NOT?? Theres even more complicated things you have to bother with if you get into c++/irrlicht/game programming stuff...

Maybe good would be to support both concepts. But perhaps best would be to have this better documented! It's so easy: 1,2,4,8,16,32,64,128... you dont even need to know that these are powers of two. Just use them. (I guess you dont even need a very high iq to continue this row of numbers )

The only disadvantage that I see is that the mask is limited to 32 types. But for most applications it should be more than enough.

It doesn't work as far as I've seen anyway. I had a wall as id=16 and a character as id=1 and the mask set to 16. Which should mean:

character = 0001
wall = 1000

And so the character should be ignored but the collision is still detected on both.

It works just perfect for me. And I just made a test with your values and I can NOT confirm your experience.

One thing should stay in mind though: the DEFAULT bit mask is -1 which is AFAIK 0x11111111 and thus always matches any mask.

If you still wanna stick with the exact match thing, then I recommend (again) this code:

Code: Select all

if (current->isVisible() && 
   (bits==0 || (bits != 0 && ( current->getID() == bits ))) 

[Tyn]

It may well be some kind of standard, I don't know and I do see your point of being able to match masks with you other ID's so you can use the differenciate between ID's rather than have them all the same. I still think exposing the user to a lower level that is unnecessary is counter productive. Having it documented would, of cause, be a great help. Having both seems the most sensible solution.

I can give you many reasons why it should be in there as well as having a mask. The main strength of the engine, as most people will probably agree, is that it is easy to pick up. It takes away a lot of the harder stuff and leaves you alone to encounter that in your own game code rather than making the graphics part any harder than it should be. It is possible to know how to program in C++ without knowing what a bit is, or even what a binary, hex or BCD number is. Why break that theme that goes through the engine for something so simple as this?

There is little reason not to simplify it and many reasons not to just leave it alone. Just because I know how to count in binary doesn't mean I should. I haven't encountered anything in C++ as of yet that makes me work things out in binary or hex unless I am debugging, even then MSVC++'s debugger is good with converting it to a decimal number for me. If you could give me an example where I would need to know how to count in binary or hex to program in C++ then I'm all ears

I don't know why it didn't work initially, maybe I did leave the ID's for some as -1, certainly possible, I have been known to do such stupid things To create a triangle selector I had to differenciate as to which mesh I was looking at anyway so I got around the problem, which is probably what I would have had to do in the first place so it's not all bad.

My view is that it having a match for integers as well as a bit mask is far more useful than having a bit mask on its own. What if I have 200 different types of node? I would rather count in decimal than binary, makes it easier to read for one thing.

[jox]

There is little reason not to simplify it and many reasons not to just leave it alone.

I see it the other way round.

Just because I know how to count in binary doesn't mean I should.

I see it this way: "Just because one doesn't know how to count in binary doesn't mean one shouldn't."

If you could give me an example where I would need to know how to count in binary or hex to program in C++ then I'm all ears

Isn't this topic an example?

I see the point of having everything easy. But please not at the expense of features/possibilities.

Well, there's probably tons of reasons for both concepts. And before it's gettin' more religious I propose the following:

Code: Select all

enum E_BIT_MASK_MODE {
	EBMM_BINARY,
	EBMM_EXACT_MATCH
}

CSceneCollisionManager::setBitMaskMode(E_BIT_MASK_MODE mode);

That should make everybody happy. Maybe there is better terms for the modes, I just quickly invented them.

Now there is still the question of the default value. I plead for EBMM_BINARY of course. Maybe we let Niko decide about that. If he likes the idea at all. I'm curious what he has to say about this anyway.

Should be easy to implement. Niko, if you want me to post the complete thing, tell me.

jox

[Tyn]

Definatly seems the best solution but up to Niko I guess

I see the point of having everything easy. But please not at the expense of features/possibilities.

I agree also, wherever possible both should be applied. The two need/should not be mutually exclusive.

[warui]

@jox:
No offence taken
Have you looked into documentation ? It's said there that:

"idBitMask: Only scene nodes with an id with bits set like in this mask will be tested. If the BitMask is 0, this feature is disabled."

So, using you example none of nodes should be picked because NT_PLAYER|NT_WEAPON = 0x000011 and there's no node that has first two bits set.

What you propose is quite easy system but with some restrictions. However what Niko proposed (and I fixed) is much more flexible.

Jox system (i will use this name for what you proposed for now to make all more readable) is very restricted by size of variable that is used to store value. Irrlicht use 32-bit integers for this, so we can have a maximum of 32 diffrent types nodes. No too many. With Niko's system we can have much more, depending on how we create ids.
For example, if we will use 8bit variable, we will get 8 diffrent types with flag system, and 28 with Niko system using only two set bits for type

Code: Select all

8-bit example
Jox's system  Niko's system (2 bits for node)
00000001     00000011
00000010     00000101
00000100     00001001
00001000     00010001
00010000     00100001
00100000     01000001
01000000     10000001
10000000     00000110
             00001010
             00010010
             00100010
             01000010
             10000010
             00001100
             00010100
             00100100
             01000100
             10000100
             ... and 10 more

We won't be able to use "|" like you (Jox) wanted, but we can use it in more flexible way.

Let's say we want four item types: normal door, locked door, locked chest (movable), chair (also movable).

In Jox's system it would be done like that:

Code: Select all

We create ids:
NORMAL_DOOR  = 0x00000001
LOCKED_DOOR  = 0x00000010
LOCKED_CHEST = 0x00000100
CHAIR        = 0x00001000

And then create following mask to be able to select...
openable items -> (NORMAL_DOOR | LOCKED_DOOR | LOCKED_CHEST)
locked items -> (LOCKED_DOOR | MOVABLE_LOCKED_CHEST)
movable items -> (LOCKED_CHEST | CHAIR)
movable locked items -> LOCKED_CHEST

Using Niko's system we will do it other way.

Code: Select all

First we create mask for object types:
OPENABLE = 0x00000001
LOCKED   = 0x00000010
MOVABLE  = 0x00000100

then ids:
NORMAL_DOOR          = OPENABLE
LOCKED_DOOR          = (DOOR|LOCKED|OPENABLE)
MOVABLE_LOCKED_CHEST = (MOVABLE|LOCKED|OPENABLE)
CHAIR                = MOVABLE

and now masks for selecting....
openable items -> OPENABLE
locked items -> LOCKED
movable items -> MOVABLE
movable locked items -> (MOVABLE | LOCKED)

And here's the trick that makes Niko's system more useful:
Let's say we want to add new item type, box which is movable and can be open.

Jox's system:
We need to add a new id - BOX = 0x00010000
And remember to change some of the bitmasks:
openable items -> (NORMAL_DOOR | LOCKED_DOOR | LOCKED_CHEST | BOX)
movable items -> (LOCKED_CHEST | CHAIR | BOX)
movable locked items -> (LOCKED_CHEST | BOX)

Niko's system:
We also need to add a new id - BOX = (MOVABLE | OPENABLE)
and then.... WOW! nothing more. All old selection mask will work with new node type. We don't need to remember to add new id to all selection bitmask !

And look at second thing. With Jox's system we needed five bits to create five diffrent id's, with Niko's we only needed three.

So, what you propose is very simple and easy method but also very restricetd. Niko's one is only a little bit harder (you just need to know how to use it effectively) but much more flexible and in result, simply, more useful.

[warui]

@Tyn
>If you could give me an example where I would need to know
> how to count in binary or hex to program in C++ then I'm all ears
Maybe I'll write somekind of tutorial about this, but i'm not promising.

EDIT: Here's something:
http://www.juicystudio.com/tutorial/c/bitwise.asp
http://www.cs.umd.edu/class/spring2003/ ... /bitI.html
http://www.somacon.com/bitops.html

@Jox
> I see the point of having everything easy. But please not at the expense of features/possibilities.
So, who's the one who want to make things easier "at the expense of features/possibilities" ?

[jox]

There is no "Jox's system". I did not invent it. Just explaining it. Actually explaining "Niko's system". Because - if you want to call it like this:

"Jox's system" == "Niko's system" == "standard bit mask system"

Your system (from the first post) would be the "exact match system". Useful too!

But your two bit system ("Warui's system" ) doesn't make sense to me.

So why not support both ideas like I proposed (CSceneCollisionManager::setBitMaskMode(E_BIT_MASK_MODE mode);

[Tyn]

I know about adding, subtracting and multiplying/dividing by two ( shifting bits left and right down a word ). Used it to create a state system in a really dumb controller the other day, really tedious. I didn't say I didn't know about it only that there is no need to go down to that level.

I didn't enjoy dealing with bits and BCD numbers and there is no need for me to ever do that again. I could convert all my numbers to BCD and do math on a hex level but why should I? Unless working on a really low level there is no need to go to that extreme and therefore no reason to enforce any bit systems on anyone.

Having the system that is currently in Irrlicht seems useful now Jox has explained it a little to me. I think a bit mask mode switcher is the best thing to use, ease of use and having functionality all in one package? Gotta be good for everyone.

Have you looked into documentation ? It's said there that:

"idBitMask: Only scene nodes with an id with bits set like in this mask will be tested. If the BitMask is 0, this feature is disabled."

Sorry for being dumb but I don't see anywhere in your post that shows that your two bit match system is right and the current one is wrong. I understand your system and it's uses but the inefficiency of using the current system is really small in terms of memory today. You are talking about possibly using one word instead of two. That really isn't going to crunch any memory away in the days of 1GB's of memory.

[warui]

@jox:
> There is no "Jox's system".
I know I just used it to make it easier to write and read.

> "Jox's system" == "Niko's system"

As i showed it's not, because:

"idBitMask: Only scene nodes with an id with bits set like in this mask will be tested. If the BitMask is 0, this feature is disabled."

So, using you example none of nodes should be picked because NT_PLAYER|NT_WEAPON = 0x000011 and there's no node that has first two bits set.

> "standard bit mask system"
Rather, "very simple example of using bitmasks". Believe me or not, bitmasks are much more useful and can be use in many more ways that you showed.

> But your two bit system ("Warui's system" ) doesn't make sense to me.
it's not two bit system. I just used two bits to show something, but ok, forget about it. Just start reading from "We won't be able to use..." and tell me what you don't understand and i explain.

So why not support both ideas like I proposed ( CSceneCollisionManager::setBitMaskMode(E_BIT_MASK_MODE mode );
I don't care. If Niko wants to added, I don't see why not, but at the moment I'll stick to make way I also suggest, changing EBMM_BINARY too something else because it sugests that my method isn't binary which is not true (actualy i used the idea you described to create ids ).
I sugest something like:

Code: Select all

enum E_BIT_MASK_MODE { 
   EBMM_ONE_OF_MATCH, // at least one of the bits set in id must match one of bit set in mask
   EBMM_EXACT_MATCH // all bits set in id must match bits set in mask
}

[warui]

@Tyn:
> Sorry for being dumb but I don't see anywhere in your post that shows that
> your two bit match system is right and the current one is wrong.
In the next paragraph. You did read it, didn't you
"So, using you example none of nodes should be picked because NT_PLAYER|NT_WEAPON = 0x000011 and there's no node that has first two bits set."

And as i said to Jox, two bits i used only to show one thing.

I understand your system and it's uses but the inefficiency of using the current system is really small in terms of memory today. You are talking about possibly using one word instead of two. That really isn't going to crunch any memory away in the days of 1GB's of memory.

It's not that way. Using what Jox presented we are restricted to only 32 diffrent types of node. 1GB of ram won't change anything. Problem lays in a size od s32 data type not size of whole ram.

And with the mathod described by Jox you will have to change your selection mask every time you add new node type and it will became bigger and bigger problem as your program grows.

[Tyn]

Yeah I read it, althogh it seems my mind was in autopilot. I thought I was missing something which is why I added "Sorry for being dumb". I may not know as much as you two seem to on the subject but I do like to try and keep up