Topic: Bad code I'm not sure about

Kiscsirke Topic Opener Joined: 2009-12-16, 12:40 Posts: 42 Pry about Widelands Location: Budapest, Hungary	Posted at: 2013-02-26, 19:54 Hi! First of all, shouldn't we have a separate "Development" forum? We have for graphic and sound development... If we had one, I'd feel more comfortable asking this there My question is the following: I've found "bad code" in logic/map.cc. It's in lines 1972-1994, and looks like this: `get_tln(fc, &neighb); if (fc.field->nodecaps() & MOVECAPS_SWIM) return true;` here the `get_xxn()` functions get a neighbour of `fc`, and put it in `neighb`. But by mistake `fc` is used afterwards instead, so `fc` is checked 7 times. So the first question is, what to do in this case? I guess just remove those lines, since it's been working like this since revision 402, back in 2003, so it's probably what's expected, and they are unnecessery lines. (The other option being of course to fix it by replacing "`fc.field->nodecaps()`" with "`neighb.field->nodecaps()`". My second question is, what do you guys expect someone like me (ergo someone who doesn't know the code good enough yet to just act on my own, and commit a change ) in this case? It's not really a bug, so I wouldn't post it as such on Launchpad. I'm not sure which change to do so I wouldn't do it as a merge request. And there's no "Development" forum here Sorry if I'm being too circuitous here, I'm just trying to improve my process Top Quote

ixprefect Joined: 2009-02-27, 13:28 Posts: 367 Tribe Member	Posted at: 2013-02-28, 07:36 Wow, nice catch. I think the reason this hasn't caused visible problems is that it is only used in an "early out" optimization in path-finding, and the only result is probably that ducks never swim onto a shore field. That would be the thing to test here. I think it should be fixed properly. There's no development forum because such issues were traditionally discussed on the mailing list. If you don't want to fix it right now, then open a bug report so it doesn't get lost. Edited: 2013-02-28, 07:37 Top Quote

Kiscsirke Topic Opener Joined: 2009-12-16, 12:40 Posts: 42 Pry about Widelands Location: Budapest, Hungary	Posted at: 2013-02-28, 16:59 You're right, fixing it does allow ducks to go to the shore! (Though it looks a bit weird ) I'll happily fix and push it myself, but won't it affect the way ships work as well? Won't there be a problem? Top Quote

ixprefect Joined: 2009-02-27, 13:28 Posts: 367 Tribe Member	Posted at: 2013-03-01, 08:03 Ships never even try to go onto a shore field, so they should be unaffected. While you're at it, you might want to change the six separate calls into a loop over the six possible directions, using Map::get_neighbour. Top Quote

Kiscsirke Topic Opener Joined: 2009-12-16, 12:40 Posts: 42 Pry about Widelands Location: Budapest, Hungary	Posted at: 2013-03-01, 15:08 Okey-dokey, done! (Woo, I was 13 years old when that bug was introduced! Crazy!) Top Quote

Adamant Joined: 2012-10-11, 15:21 Posts: 180 Widelands-Forum-Junkie Location: Alemania	Posted at: 2013-03-01, 20:47 My Pro for Development-Forum Ivan the Terrible is dead .. Genghis Khan is dead .. and I do not feel well, too. Top Quote

ixprefect Joined: 2009-02-27, 13:28 Posts: 367 Tribe Member	Posted at: 2013-03-02, 16:27 Thank you Kiscsirke (I was around 19 when I introduced that bug ;-)) Top Quote

SirVer Joined: 2009-02-19, 14:18 Posts: 1445 One Elder of Players Location: Germany - Munich	Posted at: 2013-03-09, 19:49 I think this forum is perfectly fine for coding questions. We still have and use widelands-dev, so questions could go there as well and I would not hesitate to post a bug for issues like this again - they can always be closed as invalid and code smell or semantic errors are a form of bug after all. Generally speaking - every channel is fina as long as stuff gets fixed :). Terrific finding actually! Really great to kill errors that have persisted for so long. Top Quote