hey Laarmen, just letting you know I haven't dropped off the face of the earth. I'll be in touch...

Hello laarmen,
I'm a newcomer to the game and codebase (but not new to c++ programming) and i would like tot contribute to this project.
I had a look at the code and the spec and I think I understand most of what needs to be done.
Roughly estimating, given the time I can spend on this, it will take me a few weeks to have some presentable code (I can only spend about 1 maybe 2 night per week on this and maybe some time in the weekends).
Please let me know if you want to follow up on this.

Kind regards, Pieter

@cccpxx78 Hi! We'd love to have you on board. As far as I know, @CmdrBugbear has dropped off the face of the Earth, so as far as I know this is up for grabs. (You can also find us on IRC.)

@impaktor @laarmen Hi all, I pushed some code to my fork of pioneer. Could someone have a look to see if what I'm doing is going in the right direction or is even in correct part of the code?

If general idea of what i'm doing is somewhat OK it is definitely not working code at this point, I know that at least these issues need to ironed out on a c++/project level (the logic and how it will be used is far from finished):

Some questions at this point

• locking needed or not at this level?
• reference counting using shared_ptr or the SmartPtr class of pioneer? or some other memory managment to be uses?

know issues in this version for me to investigate/fix:

• the scanner classes should not be in Sensors files, but in own file?
• second callback argument should be string not enum as in spec.
• float or double usage for the targetRange
• range based for loops OK to use? portability?
• check coding style conventions
• and much more...

Equipment are handled in Lua nowadays (used to be in C++), so the idea was to implement it in data/libs/Equipment.lua I think. @laarmen correct me if I'm wrong.

Otherwise, great to see that you've worked on this. And Lua is easy. Just some minor pitfalls for C/C++ coders, like table index starting on 1 (by default), and how nil != false.

Ok thanks for the feedback, I had the impression that what I was doing was wrong.
I'll have a look at the lua code in Equipment.lua

Yeah. The rule of thumb for this task is that if you open a cpp file,
you're doing it wrong. I'm currently on my mobile, but if you can come by
IRC later tonight, I'll be able to answer your questions.

Welcome, and kudos for picking this up.

Le dim. 10 mai 2015 à 17:17, cccpxx78 notifications@github.com a écrit :

Ok thanks for the feedback, I had the impression that what I was doing was
wrong.
I'll have a look at the lua code in Equipment.lua

—
Reply to this email directly or view it on GitHub
#3287 (comment)
.

OK, so. In my opinion the first step is to add the new slot "sensors". This should be a one liner in EquipSet.lua, plus maybe some adjustments in the ships json definitions (a scout should have lots of sensor slots).

As a side note, this issue is just about providing the equipment frame for the sensors. @impaktor, have you made an issue for your side of the story yet? :-)

@laarmen sorry I couldn't make it to IRC yesterday. I will make sure to join the channel when I'm working on this but I can't be on irc all the time (maybe in the future when I set up a VM-screen-ircII-ssh session I can be online more).
(Typically I can be online on Monday and/or Wednessday evenings)

Thank you for the input, I have taken the first step as you mentionned and pushed to my fork.
If I got it right this time, what could be the next steps (if any) for this?

creating a sensor (BodyScanner) in Equipement.lua?
As EquipType or will this require a specialization of EquipType?

I will try some things and push them to show the direction in which I'm thinking. I will clean up the commits when needed.

No problem.

So, this was the easy part.

Now, the hard part: the design. In any case, your BodyScanner object will have more methods than the typical EquipType. You have two choices.

• First, you can just create the BodyScanner as you did, and add the methods on the fly. This is the easy way, but each time we'll add a new sensor we'll have to copy-paste your code and adapt it to the new sensor's needs.
• Or, you could create a new EquipType subclass (say, SensorType) that would implement all the required methods in a generic way, letting the specifics being computed by functions passed to the constructor.

Ordinarily I would very much prefer the second way, but this issue has run a bit too long, so I'd suggest to just go with the quick 'n easy way, we merge that, and after that have another take at it and do it the "generic" way.

I cleaned up the commits and pushed to a new branch 'sensor'.
I added the interface functions directly on bodyscanner object, i.e. the easy way
These functions only have a simple first implementation that doesn't do much.
Fixed the DESCRIPTION and name confusion in the translation table.

Next steps could be:

• on BeginAcquisition get nearest body - altitude (and scan start time?)
• the updating of the progress, some time tick event somehow?
  -...

Sure, go for it.

I don't think we have up-to-date documentation anywhere on the Web, but an old version can be found at http://eatenbyagrue.org/f/pioneer/codedoc/files/LuaBody-cpp.html

About the coding style : we're a bit fuzzy on that front, esp. for the Lua side. Just try to mimic Equipment.lua coding style and you should be fine :-)

The Lua target version is 5.2. If you can write it to also be 5.3 compatible without sacrificing readability, go for it!

Hi @laarmen, I did some style cleanup in the lua.
I also added an UnPauseAcquisition method; PauseAcquisition moves the state from RUNNING to PAUSED, but I don't see when to move from PAUSED back to RUNNING? Unless the function should act as a toggle?

Something else I was thinking is that now the altitude is calculated to the center of the body (I think) should I compensate for the radius of the body? This is important maybe when the scanner has a max_range and the surface is in range but the center isn't for example.

next actions for the first step are? are there any test I should write?
PS: I will be on holiday next week, so I won't be working on this then.

for the second step, the general class:

• I would split of the state and callback stuff in the base class the rest in the specific bodyscanner
• I'm not sure what construct/style I should use to do pass in the functions. Do you have an example in the code somewhere? if not I will figure something out.
• I'll make a proposal for the functions to be passed in

This is probably not within the scope of this issue anymore, and I also think I did it the wrong way but is was a good exercise:
I also created a new branch sensor-gui, here prototyped a meterbar above the fuelbar, but in C++ WorldView.cpp. Just to have something moving with the scan on my screen :-)
I heard about the new lua gui but I don't know if/how I can use that to overlay widgets on the worldview from lua.
Concerning how I interfaced the state of the scanner to the bar; this I did with I guess a dedicated api function in C++, this is also not what you want I believe?

I need to dive into the documentation some more, any tips for some relevant wiki pages for me to read?

My comments below are without looking at the code, nor even at the
original issue, so bear with me :-)

(oh, and I might be slightly sleep deprived)

Quoting cccpxx78 (2015-05-21 19:54:26)

Hi @laarmen, I did some style cleanup in the lua.
I also added an UnPauseAcquisition method; PauseAcquisition moves the state
from RUNNING to PAUSED, but I don't see when to move from PAUSED back to
RUNNING? Unless the function should act as a toggle?

I'm guessing you made the right call. The specs were mostly a guideline
anyway. However, it might be worth documenting all this formally,
perhaps on the wiki (and of course in the code itself)? Of course,
documenting the rest of the equipment API has been buried deep in my
TODO-list for ages, so nothing new here

Something else I was thinking is that now the altitude is calculated to the
center of the body (I think) should I compensate for the radius of the body?
This is important maybe when the scanner has a max_range and the surface is in
range but the center isn't for example.

No, altitude is calculated from the surface, otherwise it would make
things REALLY difficult for those who want to refuel on gas giants.

If it ain't, I would file a bug.

next actions for the first step are? are there any test I should write?
PS: I will be on holiday next week, so I won't be working on this then.

Writing tests is something we are generally not good at. I think the
only one who has had some success with tests in Pioneer is @robn.
You can always try to do something if you're bored during your vacations
;-)

(Usually, people contribute more to Pioneer when in vacation.
@fluffyfreak is especially bad in that regard. Even when he travels, he
manages to meet up with other devs :-P)

for the second step, the general class:

• I would split of the state and callback stuff in the base class the rest in
the specific bodyscanner
• I'm not sure what construct/style I should use to do pass in the functions.
Do you have an example in the code somewhere? if not I will figure
something out.

I don't have any concrete example off the top of my head, although I'm
guessing you could find some in all the UI code.

I'd write something along the lines of that (once again, without your
code, with bogus API calls, et cætera)

bodyscanner = SensorType.New(
    {mass=1,
    ....,
    measurement_interval=5,
    onScanStart = function(self, ship)
        self.targets[ship] = {planet=ship:getClosestBody(),altitude=ship:getAltitude()}
    end,
    hasProgressed = function(self, ship)
        local target = self.targets[ship]
        return target[1] == ship:getClosestBody() and target[2]/ship:getAltitude() < 0.05 ;; Allow a 5% margin of error
    end
    })

• I'll make a proposal for the functions to be passed in

This is probably not within the scope of this issue anymore, and I also think I
did it the wrong way but is was a good exercise:
I also created a new branch sensor-gui, here prototyped a meterbar above the
fuelbar, but in C++ WorldView.cpp. Just to have something moving with the scan
on my screen :-)
I heard about the new lua gui but I don't know if/how I can use that to overlay
widgets on the worldview from lua.
Concerning how I interfaced the state of the scanner to the bar; this I did
with I guess a dedicated api function in C++, this is also not what you want I
believe?

As I said, if you write C++ you're doing it wrong ;-)
But then, I have almost no clue on how advanced we are in the new-ui
transition. @impaktor, @robn, thoughts ?

I need to dive into the documentation some more, any tips for some relevant
wiki pages for me to read?

About the UI? I don't think we have any documentation. From what I
remember, it is mostly cargo-cult style.

As I said, if you write C++ you're doing it wrong ;-) But then, I have almost no clue on how advanced > we are in the new-ui transition. @impaktor, @robn, thoughts ?

I told @cccpxx78 to do the meter bar in c++, as well as the on/off button, since that is where all other code is that does the same, so just duplicate what we have.

To go into new-ui territory is asking too much I think since none of the in-house devs cracked that nut yet, we should not scare @cccpxx78 away.

Quoting Karl F (2015-05-22 09:42:17)

To go into new-ui territory is asking too much I think since none of the
in-house devs cracked that nut yet, we should not scare @cccpxx78 away.

Oh ye of little faith!

ok, got it. I added an OnClear function too.

Feel free to open a PR for the equipment part even if you don't consider your work as ready. It will allow us to review it in a more efficient fashion (I'll tag it as WIP and assign it to myself).

The UI part should go into another part, and I'm designating @impaktor as volunteer for that one ;-)