Widelands

Erik wrote:
> If this helps, there should be an assert(sizeof(Owner_Info) == 1) 
> somewhere, so that this kind of error is detected immediately if it 
> breaks on some platform. From the gdb session it looks more like a bit 
> order problem though. Binary 10 = Decimal 2, while Binary 10 = Decimal 1 
> on my system. If this is the case, just swapping the order of is_border 
> and owner_number could perhaps help you.
Within a system there is no endianness problem. Endianness is only of 
concern when communicating with the world outside (network, files, smoke 
signals, tachyon beams, etc.)

>>
Hi,


>>
> If this helps, there should be an assert(sizeof(Owner_Info) == 1)
> somewhere, so that this kind of error is detected immediately if it
> breaks on some platform. From the gdb session it looks more like a bit
> order problem though. Binary 10 = Decimal 2, while Binary 10 =  
> Decimal 1
> on my system. If this is the case, just swapping the order of  
> is_border
> and owner_number could perhaps help you.
No, i thought so too and tried this before. I'm not very much into  
compiler, but i guess this might break on other machines as well. Is  
the performance gain really worth this hack?

Holger

> No, i thought so too and tried this before. I'm not very much into  
> compiler, but i guess this might break on other machines as well. Is  
> the performance gain really worth this hack?
As i mentioned in an earlier mail the union is unnecessary.
Bitfields work perfectly fine, IF used properly. OpenTTD for example 
quite extensivly packed bits and runs fine on x86 and POWER (also some 
others) even with both in the same network game (that's as different as 
you can get: little endian w/o alignment restrictions vs. big endian w/ 
alignment restrictions)

Holger Rapp wrote:
>> If this helps, there should be an assert(sizeof(Owner_Info) == 1)
>> somewhere, so that this kind of error is detected immediately if it
>> breaks on some platform. From the gdb session it looks more like a bit
>> order problem though. Binary 10 = Decimal 2, while Binary 10 =  
>> Decimal 1
>> on my system. If this is the case, just swapping the order of  
>> is_border
>> and owner_number could perhaps help you.
>>     
> No, i thought so too and tried this before. I'm not very much into  
> compiler, but i guess this might break on other machines as well. Is  
> the performance gain really worth this hack?
>   
Probably not, although it is indeed inside a loop. It can be changed in 
editor_game_base.cc:391:
-             m_map->get_field(f)->get_owner_info().all)
+             m_map->get_field(f)->get_owned_by())

Then the compiler would have to insert a mask operation to get only the 
7 bits.

This minor hack in Editor_Game_Base::cleanup_playerimmovables_area is 
the only reason that the union is needed. Rendering will not suffer from 
removing the union, as long as the compiler actually optimizes

x.owner_number == y.owner_number && x.is_border == y.is_border
to a single byte comparison.

But I am still wondering why your compiler did not respect the size specification when the type was bool.

Erik wrote:
> Holger Rapp wrote:
>>> If this helps, there should be an assert(sizeof(Owner_Info) == 1)
>>> somewhere, so that this kind of error is detected immediately if it
>>> breaks on some platform. From the gdb session it looks more like a bit
>>> order problem though. Binary 10 = Decimal 2, while Binary 10 =  
>>> Decimal 1
>>> on my system. If this is the case, just swapping the order of  
>>> is_border
>>> and owner_number could perhaps help you.
>>>     
>> No, i thought so too and tried this before. I'm not very much into  
>> compiler, but i guess this might break on other machines as well. Is  
>> the performance gain really worth this hack?
>>   
> Probably not, although it is indeed inside a loop. It can be changed in 
It's inside a loop!
Practically every line of code is within a loop: At least the main game 
loop.

> editor_game_base.cc:391:
> -             m_map->get_field(f)->get_owner_info().all)
> +             m_map->get_field(f)->get_owned_by())
> 
> Then the compiler would have to insert a mask operation to get only the 
> 7 bits.
oh... I'm unimpressed.
Do you know how much a x &= 0x7F costs?
How often is this executed per second? More than 10.000.000 times? Then 
maybe, just maybe, i would start to bother about performance.

> This minor hack in Editor_Game_Base::cleanup_playerimmovables_area is 
> the only reason that the union is needed. Rendering will not suffer from 
> removing the union, as long as the compiler actually optimizes
> 
> x.owner_number == y.owner_number && x.is_border == y.is_border
> to a single byte comparison.
> 
> But I am still wondering why your compiler did not respect the size specification when the type was bool.
What do you mean?
The size of a bool is implementation defined.
If it differs from the size of char the compiler won't align the 
bitfields nicely.
It's always a bad idea to put different types into a bitfield and hope 
the compiler stuffs them together.

Christoph Mallon wrote:
> The size of a bool is implementation defined.
The size of this bool is defined right there in the code:
   bool  is_border    : 1;

If the compiler can't handle that, it should at least fail and not 
silently produce something else.

Erik wrote:
> Probably not, although it is indeed inside a loop. It can be changed in 
> editor_game_base.cc:391:
> -             m_map->get_field(f)->get_owner_info().all)
> +             m_map->get_field(f)->get_owned_by())
>   
No wait. This is not a correct change. It would keep buildings on the 
border (they should be burnt). Comparing player_number against 
owner_info.all takes care of that. It was not just a performance 
optimization after all.

Erik wrote:
> Christoph Mallon wrote:
>> The size of a bool is implementation defined.
> The size of this bool is defined right there in the code:
>    bool  is_border    : 1;
> 
> If the compiler can't handle that, it should at least fail and not 
> silently produce something else.
You're getting it wrong.
The behavior the compiler shows is perfectly correct.
unsigned char and bool are totally different types. On POWER they even 
are different in size. So the compiler can't pack them in one entity. 
Even for different types which have the same size there's no guarantee.
The standard is very vague when it comes to bitfields.

struct X {
   int x:1;
};

This struct is 4 bytes in size (assuming an int is 4 bytes).
It is _not_ 1 byte (or even 1 bit).

Hi,


> Erik wrote:
>> Probably not, although it is indeed inside a loop. It can be  
>> changed in
>> editor_game_base.cc:391:
>> -             m_map->get_field(f)->get_owner_info().all)
>> +             m_map->get_field(f)->get_owned_by())
>>
> No wait. This is not a correct change. It would keep buildings on the
> border (they should be burnt). Comparing player_number against
> owner_info.all takes care of that. It was not just a performance
> optimization after all.
Doesn't matter. correcting this line would not solve all bugs  
concerning this union.


Greetings,
Holger

Holger Rapp wrote:
> Doesn't matter. correcting this line would not solve all bugs  
> concerning this union.
>   

What else have you encountered?

Am 21.09.2006 um 00:16 schrieb Erik:

> Holger Rapp wrote:
>> Doesn't matter. correcting this line would not solve all bugs
>> concerning this union.
>>
>
> What else have you encountered?
after changing this line, my hq didn't burn. but i couldn't build on  
my land (meaning my land was not my land, there was a border but no  
build help would show up and i couldn't build anyrthing)

I consider this bug setlled for now, if any more problems occure we  
rediscuss changing this union.

Holger

Christoph Mallon wrote:
> add a compile time assert to verify that the size of the struct is 1.
>   
What do you expect a compile time assert in C++ to look like? Would 
something like this hack please you?:
#define UMP_META_ASSERT(const_expr) typedef bool 
Meta_Assertion[(const_expr) ? 1 : -1]

found at: http://www.flipcode.com/cgi-bin/fcarticles.cgi?show=64445

On 9/20/06, Christoph Mallon <christoph.mallon@...> wrote:

> tachyon beams


Three questions:
- Do you have any blueprints for this hardware?
- Is there a driver in the current kernel for this?
- 10/100/1000 Mbps?

;-)

-- 
Gegroet,
Tim

Erik wrote:
> Christoph Mallon wrote:
>> add a compile time assert to verify that the size of the struct is 1.
>>   
> What do you expect a compile time assert in C++ to look like? Would 
> something like this hack please you?:
> #define UMP_META_ASSERT(const_expr) typedef bool 
> Meta_Assertion[(const_expr) ? 1 : -1]

This looks ok to me.
Though i would use extern int instead of a typedef, because in C a 
typedef must be declared at most once. (Yes, i know, there is no C in 
Widelands, but this change costs nothing)
Further i'd call it COMPILE_ASSERT.

Tim Stoop wrote:
> On 9/20/06, Christoph Mallon <christoph.mallon@...> wrote:
> 
>> tachyon beams
> 
> 
> Three questions:
> - Do you have any blueprints for this hardware?
They're not publically avaiable yet, but will be released together with 
plans for cold fusion and faster than light travel.

> - Is there a driver in the current kernel for this?
It doesn't run Linux, sorry.

> - 10/100/1000 Mbps?
Pah... petty numbers!

Christoph Mallon wrote:
> Is there are a special reason to spam my mailbox with zillions of commit 
> mails instead of doing it in _one_ commit?
>   
Yes there is. The SVN server can not handle a large commit that changes
a property of every png in widelands. Even if I try with only all the
pngs in tribes/barbarians/workers, it is too much for it. It fails with
error 500: internal server error. Seems like you just have to wear out
your delete button. Sorry for that. I remember someone else did
something similar a while ago. I also wondered the same. Now I know his
reason.

Erik wrote:
> Christoph Mallon wrote:
>> Is there are a special reason to spam my mailbox with zillions of commit 
>> mails instead of doing it in _one_ commit?
>>   
> Yes there is. The SVN server can not handle a large commit that changes
> a property of every png in widelands. Even if I try with only all the
> pngs in tribes/barbarians/workers, it is too much for it. It fails with
> error 500: internal server error. Seems like you just have to wear out
> your delete button. Sorry for that. I remember someone else did
> something similar a while ago. I also wondered the same. Now I know his
> reason.
Please submit a bug report.
Having to do a gazillion commits where one would suffice (and even would 
be The Right Thing(tm)) is ridiculous.