Functionality for removing track piece

[GoogleCodeExporter]

This change:

1) Enables the 'Remove' button when applicable.
2) Removes the track piece from the 'World' when remove is clicked.
3) Removes the track piece from the coaster instance.

I was thinking about also adding the functionality for the 'Forward' and 'Back' 
buttons on the coaster gui. Should this act the same as RCT where the piece 
blinks in white until it is removed?

Original issue reported on code.google.com by brianlta...@gmail.com on 1 May 2014 at 10:31

Attachments:

• RemovePiece.tar.gz

[GoogleCodeExporter]

Sorry I didn't see the suggested format before and I can't edit my original 
post:

What are the major things which your patch changes / adds?
1) Enables the 'Remove' button when applicable.
2) Removes the track piece from the 'World' when remove is clicked.
3) Removes the track piece from the coaster instance.

Against what version did you make your patch?
Rev 1150

Please provide any additional information below.
I was thinking about also adding the functionality for the 'Forward' and 'Back' 
buttons on the coaster gui. Should this act the same as RCT where the piece 
blinks in white until it is removed?

Original comment by brianlta...@gmail.com on 1 May 2014 at 10:51

Attachments:

• diff.txt

[GoogleCodeExporter]

Hi,

Thanks for trying to add this functionaity. Looking at the patch, you seem to 
do a few weird things. Also, the code can be improved.

- You are creating lots of almost duplicate code. Please abstract common parts 
to easy to understand functions. It will improve readability and 
maintainability of the code a lot.
- You seem to assume that CoasterInstance::pieces has a sane order. It has not, 
it's a collection of all positioned track pieces of the coaster, and is 
unordered. That means any attempt to find a related trackpiece at  
pieces[index-1] or pieces[index+1] will fail, even if it seems to work often.
- In CCW_FORWARD and CCW_BACKWARD you are removing a coaster instance from a 
voxel. Track pieces however may be bigger than a single voxel, thus in the 
general case you are not removing the entire track piece.
- Your editor settings seem to use 4 space indent. Our source code is tab 
indented. Please adjust your setting.
- Your CoasterInstance::RemovePositionedPiece function uses a different curly 
bracket style, please adjust.

Original comment by Alberth2...@gmail.com on 5 May 2014 at 10:56

[GoogleCodeExporter]

I apologize, I thought I had removed the code for the forward and back 
functions. I was just fiddling and didn't realize I left it in there.
I fixed the indents and curly brackets as you pointed out.

Original comment by brianlta...@gmail.com on 5 May 2014 at 1:27

Attachments:

• diff.txt

[GoogleCodeExporter]

No need to apologize. Coding is a complicated activity, it's easy to make 
mistakes.

The patch is a lot smaller now, so that's good :)

I am still puzzled what you're doing in the COW_REMOVE code. The "if" at line 
32 seems useless, as the while at line 33 and the condition at line 36 seems to 
take care of it.

What is the "while" loop about at 33-35? As noted before, the array is not 
ordered. This means that a positioned piece at a smaller index may actually be 
a piece that is later in the rollercoaster. If you look at 
"CoasterInstance::AddPositionedPiece" you'll see that a new piece is added at 
the first empty spot, anywhere in the array. If you remove a piece, then build 
a piece nearer to the end, the new piece may end up at the index of the just 
removed piece. Decrementing the index to find an earlier piece is not going to 
work in general, even if seems to work in your case.

Depending on what you aim to do here, you may want to use 
CoasterInstance::FindSuccessorPiece or code a FindPredecessorPiece (probably 
before removing it). This is just guessing though, as I don't understand what 
you're aiming to do.

At line 59 and 60, why do you enable forward and backward if there may be no 
current piece? Note that "SetShaded(..., true)" disables the widget rather than 
enabling, as you might think. If you leave in the "!enable", then the "enable" 
var expresses enabledness. Without the "!", the condition must be false, and 
"this->cur_piece == null" is one way to achieve that. (It's all very 
complicated double negation thinking, I am afraid. This is why I don't try to 
be smart in using "enabled" in SetShaded() rather than "!enabled", too much 
risk in messing up. I rather write a bit duplicate code instead.)

CoasterBuildWindow::RemoveTrackPiece() is not correct on several things. First 
of all, as noted before, a track piece may be longer than 1 voxel. The example 
tracks just happen to be always 1 voxel long. Just removing one of them will 
thus fail in the general case. You'll need to program something like 
CoasterInstance::PlaceTrackPieceInAdditions but remove instead of add.
Secondly, the coaster should do adding and removing of pieces to/from the world 
rather than the build window. The reason is that a ride is always responsible 
for its data in the world. This decision allows eg building or removing 
coasters from other sources than the user making a coaster, for example, you 
could load a game, or change a coaster in an in-game tutorial or so. In such 
cases, there is no opened build window.
Thirdly, after removing a track piece from the world, you are removing it from 
the rollercoaster at line 86. At this point, the latter should never ever fail. 
Please check that with an assert.

In CoasterInstance::RemovePositionedPiece, your "const" in the argument is 
fake, as you're modifying the data anyway, through this->ci->pieces[i]. I don't 
understand why you don't pass in the pointer, modify it through the pointer, 
and get the index by subtracting the array from the pointer. Wouldn't that be 
much simpler? (Although from a previous comment you may not need the index 
value any more.)

Last but not least, could you give you patches a better name than "diff.txt"? 
Some version number would be useful, so I can see from the filename which ones 
are old. Also, an indication of what it does would be useful too. I have many 
patches, I'd be going mad if they would all be named "diff" :)

(And don't despair about my walls of comment, it's normal that you don't see 
all considerations at first sight.)

Original comment by Alberth2...@gmail.com on 6 May 2014 at 5:25

[GoogleCodeExporter]

I greatly appreciate your patience with me, I know how easy it would be to just 
fix my mistakes and move on, but you point them out an give me the chance to 
learn and fix it myself, so thank you. As you can probably tell I'm new to game 
coding (and only about a year with c++) and I realize this change is a very 
small (but important) part of the game but I hope to keep learning and become a 
valuable part of the project.

With that said, I think I took care of all the points you mentioned. Hopefully 
it's "up to code", but as usual, let me know if anything needs to be changed.

Original comment by brianlta...@gmail.com on 9 May 2014 at 5:50

Attachments:

• RemoveTrackPiece-3.diff

[GoogleCodeExporter]

Hi, sorry for being late in replying, but it was a bit busy.

> I greatly appreciate your patience with me, I know how easy it would be to 
just fix my mistakes and move on, but you point them out an give me the chance 
to learn and fix it myself, so thank you.

You're welcome. It's really your patch, so you should make it work. If I took 
it out of your hands, I would write the patch, and not you. Also, I am working 
on other things (such as recolouring), so unless you make the fix, this will 
not get done until I find it on my path again.

I also enjoy doing this. Besides getting experience in explaining things to 
other people, I often see new nice solutions that I would not have thought of 
myself. For example, your re-use of FindSuccessorPiece to find a pre-decessor 
surprised me.

I know I have high code quality standards. Thanks for hanging in, and continue 
making improvements.

> As you can probably tell I'm new to game coding (and only about a year with 
c++) and I realize this change is a very small (but important) part of the game 
but I hope to keep learning and become a valuable part of the project.

It's real missing functionality that needs to be implemented. I rather have 
that you pick a smaller subject that is easy to handle for you, than aim for 
something big (say "implement FreeRCT"  :D ), and get nowhere.
Small is also good for you, it means you'll more easily make progress.

> With that said, I think I took care of all the points you mentioned. 
Hopefully it's "up to code", but as usual, let me know if anything needs to be 
changed.

You made really good progress. I agree you covered the points I mentioned, but 
not entirely in the way I imagined.
Let's run through them

+       case CCW_REMOVE: {
+           const PositionedTrackPiece *p = cur_piece;
+           this->cur_piece = nullptr;
+           for (int i = 0; i < this->ci->capacity && cur_piece == nullptr; i++) {
+               if (p->CanBeSuccessor(this->ci->pieces[i])) {
+                   this->cur_piece = &this->ci->pieces[i];
+               }
+           }

If you add "break;" in the if condition, it simply falls out of the loop after 
assigning this->cur_piece, and you don't need the complicated extra "cur_piece 
== nullptr" condition.

(now that I read my text again, I notice you don't always use the "this->" 
prefix for cur_piece. The rule is really simple, all members of the class 
should have that prefix (that is, variables defined in the class, and methods 
defined in the class. The reason is that it is immediately clear whether 
something is a local variable (eg "p") or global function, or a variable of the 
class.)

After studying this code for 5 minutes, I realized it's FindPredecessorPiece 
that you wrote here. Ideally, code should be clear enough to understand what it 
aims to do in about 5 seconds. To be honest, I wasn't exactly awake when 
reading, but the code could use more hints what it's doing.
"p" could be longer, eg "pred" says a lot more what it is than "p". Other 
options are a line of comment like "/* Compute predecessor. */".

There are even better options. If you think about the function for a bit, 
you'll hopefully agree it is nice elementary function that you can use in 
several places. (For example, here, but also in the "back" button code.) Such 
pieces of functionality are good candidates for becoming methods in a class. 
I'd propose to make this a method in this->ci, the counter-part of 
this->ci->FindSuccessorPiece.

+           for (int i = 0; i < this->ci->capacity; i++) {
+               if (&this->ci->pieces[i] == p) {
+                   this->ci->RemovePositionedPiece(&this->ci->pieces[i]);
+               }
+           }

I really don't understand why you code it like this. To me it looks like 
(assume an int x to exist between 0 and 1000)

for(int i = 0; i < 1000; i++) {
    if (i == x) printf("x=%d\n", i);
}

instead of

printf("x=%d"\n", x);

The values getting compared are a bit different (pointers instead of integers, 
but the principle is the same.
Am I missing something?

+           if (this->cur_piece != nullptr) {
+               int succ = this->ci->FindSuccessorPiece(*this->cur_piece);
+               this->cur_sel = (succ >= 0) ? &this->ci->pieces[succ] : nullptr;
+               this->cur_after = true;
+           }

If the first loop is successful, aren't you trying to find a non-existing piece 
here (this->cur_piece is the predecessor of p there, thus p is the successor of 
this->cur_piece. In the second block of code you remove that successor piece, 
and here you try to find the successor again.

(at least, I think that's what happens, but I am not sure.)

+ * Try to remove a positioned track piece from the coaster instance.
+ * @param placed positioned track piece to remove.
+ * @return If the new piece can be removed, its index is returned, else \c -1.
+ */
+void CoasterInstance::RemovePositionedPiece(PositionedTrackPiece *piece)
+{
+   assert(piece != nullptr);
+   _additions.Clear();
+   this->PlaceTrackPieceInAdditions(*piece, true);
+   _additions.Commit();
+   piece->piece = nullptr;
+}

Several things wrong here, some technicalities, and lack of predictability 
mostly.
First the technicalities.

The @param line is always followed by the name of the variable you are 
describing. To make clear where the sentence starts, we treat it as a normal 
English sentence, starting with an upper case letter (which you forgot), and 
ending with a dot (which you did).
The @return line describes the value returned by the function. In this case, 
it's a "void" function and doesn't return anything. In that case you can remove 
the @return line.
If you want to read more about these @ things, Doxygen is what you look for.

Lack of predictability is more complicated to explain. Basically, with programs 
bigger than one page, understanding the code is the hard part. When you write 
code, your aim is to help the future reader as much as possible.
The best way to do that is to be boring and dull, and extremely predictable. We 
have a hard time understanding things, but we are very good at noticing 
patterns, in particular when our predicting of the pattern does not match with 
reality.

The above function jumps out immediately for me. It is called "Remove...Piece" 
which is it's supposed functionality. However it calls "Place..Piece...". These 
words contradict each other, that can't be right.
Upon further checking, it appears that PlaceTrackPieceInAdditions is the 
culprit. It doesn't always do what it's name promises. Please make sure that 
the name of a function covers what it actually does. If you change or extend 
the functionality, the name must often change as well. (Giving a good name is 
often difficult!)

The second case of lack of predictability is about symmetry. If you have add, 
you expect to have subtract. If you can go east, you expect to be able to go 
west (if not, you're probably in a maze). If you have (the old) 
PlaceTrackPieceInAdditions, you expect its symmetric counterpart, but it 
doesn't exist. We have RemovePositionedPiece, but it does work slightly 
different.

Try to be predictable and consistent in interfaces. If you make small unique 
changes in each case, and you have 20 functions, you'll go mad. (Imagine +1 
works differently from +2, which is again different from +3 etc, now try to 
write a calculator to sum arbitrary numbers.) If they all follow the same 
principle and rules, and you have 100 functions, you're ok, as you just have to 
remember the general rule.

In this case, I'd like to see a counterpart of the old 
PlaceTrackPieceInAdditions as function in CoasterInstance. The _additions clear 
and commit should stay in the Window.
The reason for this separation is the consideration of add/removal of a 
complete rollercoaster. You can call PlaceTrackPieceInAdditions lots of times, 
and then at the end do one _additions.Commit(). Your RemovePositionedPiece 
however needs to do many small Commits.

-void CoasterInstance::PlaceTrackPieceInAdditions(const PositionedTrackPiece 
&placed)
+void CoasterInstance::PlaceTrackPieceInAdditions(const PositionedTrackPiece 
&placed, bool remove)
 {
     ....  <lots of small changes>
 }

The biggest problem here is the name of the function. If you have a good name 
that covers both functionalities, a boolean toggle is very 
nice/predictable/symmetric to have. Such a name is however often difficult to 
find. A second problem is that the "remove" toggles many values. These two 
things together makes me suggest you just make a fully separate 
RemoveTrackPieceInAdditions function. You make so many small changes, that 
sharing code doesn't really mean much any more.

(Noticed that I write "InAdditions" in the new function name and not 
"FromAdditions"? The latter would be fully symmetric, but it would not be 
correct in covering the functionality. Also, this lack of symmetry will 
hopefully trigger a careful reader into checking what it does exactly.)

While I again had a lot of comments, I do think we made great progress, we seem 
to be moving away from the more technical and faulty code problems to higher 
level considerations like scalability and predictability.

I don't know where you live, and how your Internet connection is, but I'd like 
to recommend that you visit our IRC channel #freerct. It's often much quicker 
and easier to get feedback on things by asking interactively.

Original comment by Alberth2...@gmail.com on 13 May 2014 at 6:40

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

I think I've taken care of all the issues you mentioned except for 1:

> I really don't understand why you code it like this. To me it looks like 
(assume an int x to exist between 0 and 1000)

> for(int i = 0; i < 1000; i++) {
>     if (i == x) printf("x=%d\n", i);
> }

> instead of

> printf("x=%d"\n", x);

> The values getting compared are a bit different (pointers instead of 
integers, but the principle is the same.
> Am I missing something?

This has actually been confusing me from the beginning. It's probably because 
I'm still getting used to working with pointers (I've been spoiled with .Net 
programming). At first I was passing this->cur_piece (as a const) to 
RemovePositionedPiece() and you said I shouldn't do that because the const is 
"fake". What would the correct way to pass this->cur_piece to that function? Or 
should I be using a different approach to removing the piece from the 
CoasterInstance?

Also I checked the IRC and it seemed to be a ghost town. However I'm on Central 
time so maybe it's more active during another part of the day.

Original comment by brianlta...@gmail.com on 14 May 2014 at 1:07

[GoogleCodeExporter]

Ah, right.

My comment was intended against use of "const", not against passing in a 
pointer or making a change through this->cur_piece.

"const Foo *" says to the programmer "I promise the data it points to will not 
change". If you then change it anyway, with a work around by first looking up 
the element it points to and then change it through the element, I think you're 
breaking your promise, even though technically you are not.

The solution in this case in my opinion is to be honest to the programmer, and 
not make a "const" promise, ie use "Foo *" instead, which says "it may or may 
not change".

It could be that the channel is quite empty; the program is not playable yet, 
so no users. Activity is normally in the evening central european time, and in 
the weekends.

Original comment by Alberth2...@gmail.com on 14 May 2014 at 4:33

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

So you're suggesting to just change this->cur_piece to non const?

Original comment by brianlta...@gmail.com on 14 May 2014 at 5:39

[GoogleCodeExporter]

looks that way

Original comment by Alberth2...@gmail.com on 14 May 2014 at 6:04

[GoogleCodeExporter]

OK this should do the trick.

Original comment by brianlta...@gmail.com on 15 May 2014 at 4:52

Attachments:

• RemoveTrackPiece-4.diff

[GoogleCodeExporter]

added in r1167 and r1168. Thanks!

Original comment by Alberth2...@gmail.com on 15 May 2014 at 6:23

• Changed state: Fixed