45° rotated rectangle clearing / leveling patch

[NukeBuster]

The compiler warnings about viewport.cpp are now fixed, Thanks to Eddi(irc, don't know forum nick.)

Please test this patch. Also please give me some feedback on the code.

Patched against r10414

[r_a_s_robin]

Is there anyway to apply this patch to 0.5.2?
I'm not a code expert but i have seen that some copy-paste tool could just be installed without compiling or anything like that. Maybe something like that is possible for this great patch as well?

[belugas]

Is there anyway to apply this patch to 0.5.2?
I'm not a code expert but i have seen that some copy-paste tool could just be installed without compiling or anything like that. Maybe something like that is possible for this great patch as well?

A patch, in this context, means a difference on the source code level. Not an executable that can modify the binary of the 0.5.2.
So, no, this cannot be applied without a full recompilation of the code-source of 0.5.2.
Furthermore, 0.5.2 is an older version of the code, still written in C, while this patch is aimed for C++ in trunk. So, unless the author adapt it for the 0.5.2 environnement, not a chance.

[r_a_s_robin]

When will it be in the nightly versions?

[Sacro]

When will it be in the nightly versions?

It's got to be added to trunk first, then you have to wait until 19:00 GMT for the nightly to be built.

[belugas]

When will it be in the nightly versions?

When we decide it can be included in trunk.
When we think it is ready for trunk.
When we are sure it will not break anything.
When we'll have time to evaluate the code and
when we'll have time to merge it.

[NukeBuster]

Just an revision update.

Flyspray

[belugas]

Little note :

Code: Select all

+	if (((fx >= 0 && xx <= fx && xx >= 0) || (fx < 0 && xx >= fx && xx <= 0)) &&
+			((fy >= 0 && yy <= fy && yy >= 0) || (fy < 0 && yy >= fy && yy <= 0))) {
+		return true;
+	} else {
+		return false;

could very well be cleaner with

Code: Select all

+	return ((fx >= 0 && xx <= fx && xx >= 0) || (fx < 0 && xx >= fx && xx <= 0)) &&
+			((fy >= 0 && yy <= fy && yy >= 0) || (fy < 0 && yy >= fy && yy <= 0));

Another note :

Code: Select all

ex >= sx ? x += 1 : x -= 1;

could be made the same as

Code: Select all

curh += (curh > h) ? -1 : 1;

If you see what i mean

Watch out, there are roaming tabs here and there

I still like that patch

[NukeBuster]

I've implemented your coding suggestions and am looking through the code to eliminate all the instances.

There is one thing troubeling me, there are 3 or 4 parts in the code that are almost the same... I'm looking into if it is worth making a function.

[Bilbo]

I've implemented your coding suggestions and am looking through the code to eliminate all the instances.

There is one thing troubeling me, there are 3 or 4 parts in the code that are almost the same... I'm looking into if it is worth making a function.

Definitely. Though if it is called often, it may possibly be worthy to declare it as inline function, which will essentially instruct the compliler to fut the function code directly where the function was called, saving the call.

[belugas]

Might be a big inline... We are not too fund of big inline functions...
And definitively not macros too

[Bilbo]

Well, if it is big, then make it ordinary function - not inline. Basically, the difference in source is just having/not having inline keyword (+ some conventions that inline functions have to be in headers, non-inline not (only declaration should be there), which can be solved by simply moving the function around :)), though the generated binary is then quite different fopr inline/non-inline case;)

Inline is good for very small functions like "return m[x].something" where the call overhead would be large.

[NukeBuster]

The function will replace code in viewport.cpp, landscape.cpp and cmd_clear.cpp in which file should the function reside?

[Bilbo]

Depends on what the function does - it should be put near "similar" functions. If unsure, I'd stick the function body probably in landscape.cpp, and its declaration in landscape.h as I think it would be probably the correct place for it. It can be easily moved if necessary, after all :)

[mamuf]

Is this patch network-compatible with vanilla nightly?

[Bilbo]

Is this patch network-compatible with vanilla nightly?

As I looked in the patch - since it modifies the network protocol for leveling and clearing area by adding flag for dialognal clear ... no. Unfortunately, it is not network compatible - i.e. both you and server either need to have or have not the patch.

[NukeBuster]

Thanks for the help on the function. I am still looking into that.

Updated the patch to the latest nightly(r10684). No new features or big code changes were added.

[belugas]

Looks really better
But indeed, still that code duplication that is irritating (not your fault, by the way)...

[CobraA1]

could very well be cleaner with

Shorter yes - I dunno about "cleaner" though - his code makes the return value obviously a boolean, and your code is a bit less obvious.

[Aydan]

Little note :

Code: Select all

+	if (((fx >= 0 && xx <= fx && xx >= 0) || (fx < 0 && xx >= fx && xx <= 0)) &&
+			((fy >= 0 && yy <= fy && yy >= 0) || (fy < 0 && yy >= fy && yy <= 0))) {
+		return true;
+	} else {
+		return false;

could very well be cleaner with

Code: Select all

+	return ((fx >= 0 && xx <= fx && xx >= 0) || (fx < 0 && xx >= fx && xx <= 0)) &&
+			((fy >= 0 && yy <= fy && yy >= 0) || (fy < 0 && yy >= fy && yy <= 0));

Shorter yes - I dunno about "cleaner" though - his code makes the return value obviously a boolean, and your code is a bit less obvious.

What's the return value of || or && ? Obviously boolean, since they're logic operators, so no need for the if. Everything else is just cosmetics i think.