resurrection of diagonal level and clear patch

[fonso]

Below you can find the patches diagonal levelling. The "tileiter" patch only implements the tile iterators which can be reused for other purposes. It also rewrites the normal levelling and clearing methods to use the orthogonal iterator. The "diaglvl" patch implements the actual levelling and clearing on top of the diagonal iterator. For convenience I also provide an all-in-one patch implementing both. Thanks for your feedback.

You can also pull them from my git repository at git://github.com/fonsinchen/openttd-cargodist.git
Each patch has its own branch. The branches are called "tileiter" and "diaglvl".
An up-to-date all-in-one patch at at http://github.com/fonsinchen/openttd-ca ... aglvl.diff
It applies to the trunk version given in http://github.com/fonsinchen/openttd-ca ... ERSION.txt

[Zuu]

I'll try.

One argument can be that there are people that don't know of all fancy CTRL-options available in OpenTTD. If you follow the development of OpenTTD you'll learn the new CTRL behaviors one after another but if you are new to OpenTTD it can take long time to find all various nice features that are "hidden" on the CTRL-key.

I don't know if anyone have vented any such comments in the previous thread but sometimes people do vent such things on patches. But add a note that CTRL toggles diagonal operation in the tooltip of the affected buttons in the landscape toolbar and it should be enough I think. (Let's leave out the discussion about if tooltips should be shown automatically or not)


EDIT: I did my homework and read through two old thread on the subject and only found one argument. That there is another patch using CTRL but it was argued that diagonal leveling is more useful and therefor should use CTRL. http://www.tt-forums.net/viewtopic.php?p=668831#p668831

[fonso]

Ah, I see, thanks. Somehow I didn't realize they meant the drag&draw patch. Why does the drag&draw patch need the CTRL key anyway? Would anyone notice if drag&draw became the default behaviour for raise and lower? I mean, it's pretty much what you'd expect when keeping the mouse button pressed and moving the cursor around, isn't it? Like that only explicit levelling would actually do levelling. At the same time I wouldn't have to care about diagonal raise and lower. And as an additional benefit we'd get rid of the problem that raise can lower terrain in certain situations (and the other way round). Perhaps we should have a poll: Who is actually using area raise and lower instead of raising or lowering one tile and then using level from there?

We still need a solution for drag&draw levelling then ... any ideas? Do we need it?

[Roujin]

Well, I've been trying my luck with the devs about my drag&draw terraform before, but they didn't seem too interested.. even when I tuned it down to a scedit-only version that doesn't use CTRL, after one of them proposed that to me (they seem reluctant to add stuff that uses CTRL), I got no more feedback after that.

Sorry for having not updated it for a while, been busy with university lately...

edit: okay, I have to withdraw the part about no answer.. SmatZ wrote an answer in my flyspray task 2 days ago.

[fonso]

A somewhat simplified version of the same patch. Does the same thing, but eliminates the if-else ugliness. The issue with consistency and CTRL usage isn't resolved due to lack of better ideas. Perhaps I should take a look at that Flyspray thing, too ...

[planetmaker]

Who is actually using area raise and lower instead of raising or lowering one tile and then using level from there?

I do . But that's indeed something I could live without, if I trade diagonal terraform tools for that

[fonso]

New version with some cosmetic changes.

[Wolf01]

Who is actually using area raise and lower instead of raising or lowering one tile and then using level from there?

I do . But that's indeed something I could live without, if I trade diagonal terraform tools for that

I would trade both the diagonal etc tool and the raise/lower area tool for the "paint" (drag&draw) tool

[fonso]

renamed some variables as requested. Fixed some indentation problems.

[fonso]

new version with iterators instead of duplicated code, flags for leveling modes and cosmetics.

[Yexo]

Nice patch, I hope this'll make it to trunk someday.

Some small comments about the style:

Code: Select all

+	if (p2 & LEVEL_RAISE)
+		h++;
+	else if (p2 & LEVEL_LOWER)
+		h--;

You should use brackets if you put h++ on a different, and always for else statements. So it'll become.

Code: Select all

+	if (p2 & LEVEL_RAISE) {
+		h++;
+	} else if (p2 & LEVEL_LOWER) {
+		h--;
+	}

You do this some more times in your patch.

Code: Select all

DoCommandP(end_tile, start_tile, _ctrl_pressed, CcPlaySound10, CMD_CLEAR_AREA | CMD_MSG(STR_00B5_CAN_T_CLEAR_THIS_AREA));

Looks like you forgot to change _ctrl_pressed to _ctrl_pressed ? LEVEL_DIAG : 0 on this line, and for CmdClearArea in general.

[belugas]

Sharp eye Yexo
Let say that this patch has never been closer to trunk inclusion

[fonso]

Thanks Yexo.

The brackets thing was unknown to me. I know I should memorize the coding style better .

[fonso]

Next version with all if-else bracketified, where needed and CmdClearArea also uses LevelMode (although I could argue about that ...).

[peter1138]

I don't think LevelMode should be declared in command_type.h.

Another minor code-style thing; can you place the { after a function on a new line. There are still some missing braces around some if () statements.

[fonso]

new version with (hopefully) all the brackets correct now, LevelMode divided into LevelMode and Orientation, iterating with *Iterator::end.

Edit: forgot the brackets at function start; corrected now.

[fonso]

To sum it up:

- blathijs likes it as it is now
- peter1138 finds the *Iterator::end confusing because it contains "random values"
- hylje considers the whole idea of C++ iterators braindead
- others are silent

I don't care. I'll happily make a java style iterator of it, or change it back so that you have to dereference it to get the end condition or insert a special "end" flag somewhere or even make a common base class which provides an empty "end" member or whatever. But I'm at a loss now. Please comment.

[fonso]

ok, now I have two alternative versions. One with java-style iterators which is very concise and clean and one with more elaborate c++-style iterators which provides an extra class InvalidIterator, so that I don't need to fill the invalid iterator with random stuff.

[RSpeed tycoonfreak]

I like this idea, can i see a screenshot please?

[fonso]

I like this idea, can i see a screenshot please?

Sure. Here are some. Forgive the strange screenshot windows ... I'll have to investigate that.