Oolite Bulletins

Eric, what you are saying, you raise the counter-argument that the way NPCs are scripted, it is implied that they have no "bounty hunter scanner" add-on, so it would be an unfair advantage if players could have one, do I get that right?

another_commander wrote:

I just want to clear up a possible misunderstanding here. ... rework your submission to make it trunk-ready. You will find its chances of inclusion will be substantially increased.

Sorry for posting twice in a row, but that does mop away a misunderstanding or three.

Seems this debate is a result of talking to each other via the cold dark medium of the internet and all that

another_commander wrote:

b) Are complete, ready-to-roll-in patches. Sometimes, when a submitted idea is really good, we might put some work of our own to make it mainstream ready and

Screet: In my personal opinion, your patch fails on the second point. The code changes you proposed earlier in this thread are not enough for inclusion in the standard game. They are hacks. We normally do not spread unknown and non-standard equipment names in the code, hoping that if the player has such equipment it will work. For this to be able to go in, it must also have JS visibility, as stated in one of my previous messages.

The equipment plist part could well be contributed as OXP without which the simple change in the code would not come to action.

JS can detect, add and remove equipment by calling their names, thus I do not understand what is missing from a JS perspective. If you want that this eq does not work by changing the code, but only as a JS addon, then it won't be possible to do so, as the coloring and scanner functionality is hardcoded.

Screet

I believe they are wanting the functionality to be JS-accessible without the prescribed equipment. That way, should someone want to leverage this functionality in some way, the OXP can do so directly without having to award a piece of equipment. They would need the functionality to be toggle-able via a full javascript property. That way, should another author want to use the functionality in another piece of equipment they design, they are not limited to awarding their equipment and your's - They can simply have their equipment turn on the property.

As to how you go about doing that, I do not know. I am still learning Javascript, and haven't moved on to Objective-C yet.

If my interpretation is wrong, a_c, please correct me.

Screet wrote:

JS can detect, add and remove equipment by calling their names, thus I do not understand what is missing from a JS perspective. If you want that this eq does not work by changing the code, but only as a JS addon, then it won't be possible to do so, as the coloring and scanner functionality is hardcoded.

I think when A_C means is that you must not check against the equipment in your code but against a newly defined variable that is set to false by default.

That variable than must be accessible by JS. The JS script than becomes responsible to turn that property on/off when buying/destroying the equipment.

You may want to see how the target sensitive reticle functionality is introduced in a totally transparent way in the code and then becomes an OXP equipment by Eric's script for an example of what I mean.

Screet, that looks to me like a pretty clear example of what kind of integration a_c is referring to.

And yes, what Eric said. It might not really make much difference to you, but the code you showed us as is quite inefficient, you really shouldn't need to call hasEquimpentItem item every time you redraw the screen. Querying an internal boolean is much much faster.

Even if this still doesn't quite sound right to you, you'll have to trust us that efficient code is preferrable to inefficient code - I'm sure pmw57 will be more than willing to go through the differences between the two!

Kaks wrote:

Even if this still doesn't quite sound right to you, you'll have to trust us that efficient code is preferrable to inefficient code - I'm sure pmw57 will be more than willing to go through the differences between the two!

I wrote myself, when I did show that version:

Screet wrote:

I was worried about a possible performance impact by calling hasEquipmentItem instead of setting a variable if the player does have the equipment and resetting it upon load/ship change/equipment damage/equipment selling, but at least on my machine it's not noticeable and I don't know yet where these variables would have to be reset.

I've improved existing C++ code to run in fractions of a second instead of taking more than 10 minutes while at the same time to provide better results.

With C++ I'm pretty good at optimizing for speed, but when it's not appearing to be necessary and doing that optimization also results in a much more complex code that has reduced readability and could cause further changes being made to be more complex (or make creation of bugs more easy), then I'm staying with slower code.

That's why I wrote that I was worried about that myself and already did outline how it could be improved.

Thus, if that IS the remaining problem, why wasn't it written so? I also still await an answer whether what you wrote IS what A_C is asking for.

I'm not going to make changes when I don't know whether I have to throw them away later on. It's just that I offer(ed) to share something which works fine for me. Thus, for me alone, there's no need for change. I would do it if I know that it is what is required to allow others to use this feature too, if they want it.

I've also got a little bit of difficulty to understand why it's required that NPCs with a bounty on them can be excluded from showing up on that enhanced scanner as having a bounty. Thus, it's also the question if it's really necessary to add additional code for that. The reason is that I don't see any point where it would break something when the player does know that there does exist a bounty on some ship.

Screet

Kaks wrote:

you really shouldn't need to call hasEquimpentItem item every time you redraw the screen. Querying an internal boolean is much much faster.

If that is the reason why the proposed change was called a hack, then please admit that Oolite simply has been hacked together.

I've just had a short look at the code because I did remember seeing the exact same behaviour by original code earlier.

Turns out that Oolite code is written in many instances which do update for each time the screen is redrawn the same way: It checks for existing equipment instead of using booleans.

Screet

Screet wrote:

Kaks wrote:

you really shouldn't need to call hasEquimpentItem item every time you redraw the screen. Querying an internal boolean is much much faster.

If that is the reason why the proposed change was called a hack, then please admit that Oolite simply has been hacked together.

I've just had a short look at the code because I did remember seeing the exact same behaviour by original code earlier.

Turns out that Oolite code is written in many instances which do update for each time the screen is redrawn the same way: It checks for existing equipment instead of using booleans.

Screet

Since you have to ask, it was not called a hack because of invoking hasEquipment each frame. As far as I am concerned, it is not a matter of performance, it is a matter of code organization, cleanness and maintainability. What you do with your change is take the easy way out: You are using a check of a non-existing piece of equipment in the code to execute an action depending on whether the player has it provided by an OXP or not. This is not the way things are done in Oolite. What would happen inside the code if every person with an idea about new equipment started throwing in random equipment names and implementations similar to this? It would become impossible to know which equipment is internal and which is supposed to be coming from outside. Then you go as far as suggesting the inclusion of a jammer for your equipment(!) Well, no. I invite you (again) to check the target sensitive reticle implementation. If you can do this for your kit then fine, if not, sorry but it does not go in like this. I am not a professional programmer and yet I can tell you that this is one of the most blatant hacks that I have ever seen. And yes, I have had my fair share of hacks submitted in the code base and every time something was out of line I was getting the red card by Ahruman or the other devs. As far as I am concerned, this is not ready for prime time. Once you have improved it to actually be a transparent implementation, we can see what we can do.

Perhaps you two (A_C & Screet) could simply clarify the dev requirement side (how exactly should this be implemented) and the requesting side (what you, Screet, exactly want the new code to do inside Oolite) via PM?

I don't claim to understand more than every second word you're using, but perhaps taking this out of a public debate and actually trying to understand what the other one means might help take the steam off a little? I am getting the feeling this thread is suffering from misunderstandings on all sides, that could get resolved ...

Just my 0.2Cr, perhaps this can just be solved by the smallest common denominator (dev-approved code first, but if that is done then the change goes in?)

*ducks under thrown objects & runs*

L

another_commander wrote:

Since you have to ask, it was not called a hack because of invoking hasEquipment each frame. As far as I am concerned, it is not a matter of performance, it is a matter of code organization, cleanness and maintainability. What you do with your change is take the easy way out: You are using a check of a non-existing piece of equipment in the code to execute an action depending on whether the player has it provided by an OXP or not. This is not the way things are done in Oolite.

The cleanness and maintainability of that little change I proposed I think is far better than that of target reticle. Indeed, target reticle should IMHO be inside the code itself, as all the JS does is to provide some workaround for functionality that requires code in oolite anyway.

The equipment is not non-existing as I did provide the part which should go into equipment.plist - the wormhole scanner did go the same way. Only if people do not want that such additional eq can be bought in normal Oolite (although it requires code in Oolite) it is required to add that in an additional oxp. I think that's strange.

Thus it appears to me that this is handled by double-standards. If it's liked in general, it's put in, if not, it has to be an oxp workaround and only maybe the required code change is done to Oolite.

another_commander wrote:

Then you go as far as suggesting the inclusion of a jammer for your equipment(!) Well, no.

I was asked to provide a way to make exclusions for special ships possible. That I did. I could also add an equipment.plist entry for that one, but it appears that this would not be liked.

Thus, I leave things as it is.

Everyone who wants to have such a modification (either by equipment or direct ability for the normal scanner) can use the info I did provide earlier and change their sources accordingly. It's very simple to add and does not require coding skills. I am not writing a JS workaround for this. If anyone would like to do that, feel free to do so.

Screet

<Opens door on thread>
<notices flames licking around the furniture>
<closes door on thread>
<walks away nonchalantly, whistling as he goes>

Screet, if you want to leave things as it is, that is fine with me. Maybe one of the other developers will be willing to commit your change to the trunk. I will not, at least in its current state. I think that arguing about a color change has already gone too far.

Kaks wrote:

Screet, that looks to me like a pretty clear example of what kind of integration a_c is referring to.

And yes, what Eric said. ...<snip>... you really shouldn't need to call hasEquimpentItem item every time you redraw the screen. Querying an internal boolean is much much faster.

Please notice the bits in italics, emphasised here for extra clarity.
Hmm, I don't know why I didn't notice 'hasEquipmentItem item' before.

Kaks wrote:

Kaks wrote:

Screet, that looks to me like a pretty clear example of what kind of integration a_c is referring to.

And yes, what Eric said. ...<snip>... you really shouldn't need to call hasEquimpentItem item every time you redraw the screen. Querying an internal boolean is much much faster.

Please notice the bits in italics, emphasised here for extra clarity.
Hmm, I don't know why I didn't notice 'hasEquipmentItem item' before.

You know, Oolite will need refactoring then. It itself does behave this way. It's doing this both for player and NPC ships MANY times per frame.

If it were C++, it probably would not be an issue if that method does use a map, but I did not have a look yet at how speedy it is in oolite. Are there any performance results from a profiler?

Anyway, it seems that Oolite does add code for equipment in ship and playership code instead of having equipment properly work as objects (which might come from a speed concideration vs. object orientation) which raises the question as how much refactoring there should be and which way oolite would best profit from it. As I said, Oolite does it, many many times per frame, probably several thousand calls to hasEquipment item per second. Looks like Oolite could do well with some refactoring there if it's slowly accessing that sort of thing and probably in order to make that code more object orientated.

Screet