Author Topic: Some bugs find in code  (Read 8656 times)

Offline Yankes

  • Global Moderator
  • Commander
  • *****
  • Posts: 3350
    • View Profile
Some bugs find in code
« on: April 27, 2018, 06:02:36 pm »
Doing other work I find couple dodgy parts in this function implementation.

First it not support multi purpose buildings because of `else if` (it will ignore cleanup of other functions)

Another is usage of `(*facility)->getCraft()` that look it should only be used for display purpose. What will happen when you use multiple crafts per hangar or craft is on mission when base was attacked?

[ps]
`getUsedHangars()` is call for each `_productions` even if do not need this value because type `getCategory()` is not craft. This is very heavy function and in each iteration value is same (until you remove something but then `break;` is done and you do not call it any more).
« Last Edit: April 27, 2018, 10:35:32 pm by Yankes »

Online Meridian

  • Global Moderator
  • Commander
  • *****
  • Posts: 9098
    • View Profile
Re: Bugs in Base::destroyFacility
« Reply #1 on: April 27, 2018, 06:14:39 pm »
First it not support multi purpose buildings because of `else if` (it will ignore cleanup of other functions)

I found this a few months ago too... and discussed with Warboy on Discord.
I don't remember what we agreed... probably we just forgot about it again :)
I'll have a look in the Discord history and post an update later..
But a small fix and PR to vanilla wouldn't hurt.
I'm sure I have it somewhere on my todo-list still.

Another is usage of `(*facility)->getCraft()` that look it should only be used for display purpose. What will happen when you use multiple crafts per hangar or craft is on mission when base was attacked?

Here, I don't have any first-hand info... but I don't think OpenXcom (intentionally) wanted to support multiple craft per facility... unless SupSuper/Warboy say otherwise I assume it's just an unfortunate side effect of int variable instead of boolean variable...

Offline Yankes

  • Global Moderator
  • Commander
  • *****
  • Posts: 3350
    • View Profile
Re: Bugs in Base::destroyFacility
« Reply #2 on: April 27, 2018, 06:49:25 pm »
I found this a few months ago too... and discussed with Warboy on Discord.
I don't remember what we agreed... probably we just forgot about it again :)
I'll have a look in the Discord history and post an update later..
But a small fix and PR to vanilla wouldn't hurt.
I'm sure I have it somewhere on my todo-list still.
Yup, I already change this on my working copy, when I will have some free time I will push it to master.

Here, I don't have any first-hand info... but I don't think OpenXcom (intentionally) wanted to support multiple craft per facility... unless SupSuper/Warboy say otherwise I assume it's just an unfortunate side effect of int variable instead of boolean variable...
Probably, but even if it only one craft this code is still wrong (if I read it correctly).
Consider case:
You send craft to intercept ufo but meanwhile your base was attacked by aliens and they destroyed your hangar.
Now what will be returned by `getCraft()`? I except null, this mean craft will not be destroyed and it will try return to base. If you have other free hangars then it will probably work fine but if it was last one?

Online Meridian

  • Global Moderator
  • Commander
  • *****
  • Posts: 9098
    • View Profile
Re: Bugs in Base::destroyFacility
« Reply #3 on: April 27, 2018, 08:27:14 pm »
If you have other free hangars then it will probably work fine but if it was last one?

Maybe it was done so because of "airborne transfers" advanced option?
Just guessing...

Offline Yankes

  • Global Moderator
  • Commander
  • *****
  • Posts: 3350
    • View Profile
Re: Bugs in Base::destroyFacility
« Reply #4 on: April 27, 2018, 09:56:49 pm »
Another possible bug:
https://github.com/SupSuper/OpenXcom/blob/61caccdba9bbc6ea76aa1cb5dee31bfc7a1e5967/src/Savegame/Production.cpp#L215
Should craft be deleted before vector erase?

Another: https://github.com/SupSuper/OpenXcom/blob/61caccdba9bbc6ea76aa1cb5dee31bfc7a1e5967/src/Savegame/Production.cpp#L101
It count transfer crafts too, even if can't use them.

`removeResearch` and `removeProduction` do not delete object, sometimes is deleted outside but there are places where is not.
« Last Edit: April 28, 2018, 12:25:19 am by Yankes »

Offline SupSuper

  • Lazy Developer
  • Administrator
  • Commander
  • *****
  • Posts: 2162
    • View Profile
Re: Some bugs find in code
« Reply #5 on: May 03, 2018, 02:21:42 am »
Here, I don't have any first-hand info... but I don't think OpenXcom (intentionally) wanted to support multiple craft per facility... unless SupSuper/Warboy say otherwise I assume it's just an unfortunate side effect of int variable instead of boolean variable...
facility->getCraft() is an ugly hack for the right-click facility feature (crafts shouldn't belong to facilities) and I personally don't think it should be relied upon outside of this purpose.

Another possible bug:
https://github.com/SupSuper/OpenXcom/blob/61caccdba9bbc6ea76aa1cb5dee31bfc7a1e5967/src/Savegame/Production.cpp#L215
Should craft be deleted before vector erase?

Another: https://github.com/SupSuper/OpenXcom/blob/61caccdba9bbc6ea76aa1cb5dee31bfc7a1e5967/src/Savegame/Production.cpp#L101
It count transfer crafts too, even if can't use them.

`removeResearch` and `removeProduction` do not delete object, sometimes is deleted outside but there are places where is not.
This is what happens when I listen to feature requests, better let the experts handle them. :P I'd appreciate any PR fixes.

Offline Yankes

  • Global Moderator
  • Commander
  • *****
  • Posts: 3350
    • View Profile
Re: Some bugs find in code
« Reply #6 on: May 03, 2018, 02:41:06 am »
Bad thing is I find this bugs during refactor in my branch, I could post commit as reference but it could be hard to apply it to master.

[ps]

It was not so bad, I did not merge bug fixes and refactor in one commit.
Possible candidates for cherry-pick:
https://github.com/Yankes/OpenXcom/commit/ae52896c4e6f85abef3f1d590034bb658cbdec60
https://github.com/Yankes/OpenXcom/commit/f256020a7d69c2462e08f261b4c7d4402c6e1093
https://github.com/Yankes/OpenXcom/commit/ff101851b089e20e6242a8dab4502355d199088a


Commit for reference (have refactor mixed in):
https://github.com/Yankes/OpenXcom/commit/e86024d5414eea92ddd473f20a1c2484c375f30d
https://github.com/Yankes/OpenXcom/commit/6ab89a1bbae4a7e4f484eef3bb93c2b7c220f566

Overall in this branch I keep my WIP that have fixes:
https://github.com/Yankes/OpenXcom/commits/stash/workInProgress

« Last Edit: May 03, 2018, 03:05:11 am by Yankes »

Online Meridian

  • Global Moderator
  • Commander
  • *****
  • Posts: 9098
    • View Profile
Re: Some bugs find in code
« Reply #7 on: May 03, 2018, 10:54:07 am »
That's a lot of refactoring... looking forward to merging that  :-\

Offline Yankes

  • Global Moderator
  • Commander
  • *****
  • Posts: 3350
    • View Profile
Re: Some bugs find in code
« Reply #8 on: May 03, 2018, 02:59:08 pm »
That's a lot of refactoring... looking forward to merging that  :-\
I can always help with this, reorganize commits in that way two biggest offenders will be first, and then merge it my self with your branch.
Overall I did this refactors to simplify work in future, right now in some cases same logic is spreed in multiple of places and if you want alter it you would need modify each place separately (example could be unloading vehicles). Even worse, sometimes each place do not work in same way.
Another problem I fix is ineffective code, like using `std::string` in rules classed interfaces and then searching for correct object in `Mod` object.
Problem is that is very costly to search map using strings (`2 * log(n)` memory visits, that is huge) and result will never change. simply better is to cache relation at mod load, with this access to relation will be lot faster and use less memory. Another benefic is that error will be generated at load not when wrong relation is used during playing. This will be big grain for mod creators too.

Online Meridian

  • Global Moderator
  • Commander
  • *****
  • Posts: 9098
    • View Profile
Re: Some bugs find in code
« Reply #9 on: May 03, 2018, 04:57:47 pm »
No worries, I'm just an old grumpy man complaining :) Ignore me.

I need to read and understand your code anyway, so that I know about it when I do my stuff... and merging will force me reading it :)

Online Meridian

  • Global Moderator
  • Commander
  • *****
  • Posts: 9098
    • View Profile
Re: Some bugs find in code
« Reply #10 on: May 10, 2018, 11:38:47 am »
Possible candidates for cherry-pick:
https://github.com/Yankes/OpenXcom/commits/stash/workInProgress

PR for vanilla: https://github.com/SupSuper/OpenXcom/pull/1184

I have replaced nullptr (requires c++11) with 0 and solved a few conflicts (variable names).
Also added one commit of my own at the end.

PS: The best thing though... there were no CRLF errors when I was cherry-picking... happy panda :)

Offline Yankes

  • Global Moderator
  • Commander
  • *****
  • Posts: 3350
    • View Profile
Re: Some bugs find in code
« Reply #11 on: May 12, 2018, 08:56:53 pm »
Thank for your work :)