another way to prevent stacking of shields & detectors

[Dilvish]

Cjkjfnby was just asking me about why some of the AI code figures out attack values in a different way than it gets shield values, and I realized that's another important reason why it would be best for the capacity for these parts to actually be the basic value-- so that when the AI is evaluating ship designs it can actually retrieve the basic value for a Defense Grid rather than having to rely on a hardcoded lookup that Defense Grid has a basic value of 4. When dealing with actual ships the AI can of course just rely on the actual meter values, but the AI needs to be able to assess designs as well.

Of course for now at least it still needs hardcoded values for the species bonuses, but the AI could be more adaptable to a broader range of user customization if it could directly retrieve shield part values for designs. Ideally the stackability characteristic would also wind up being in the part specification like has been suggested in this thread, but even if the AI has to rely on assumptions about stackability it would still be better off not having to rely on assumptions about basic part values.

[Geoff the Medio]

...important reason why it would be best for the capacity for these parts to actually be the basic value-- so that when the AI is evaluating ship designs it can actually retrieve the basic value for a Defense Grid rather than having to rely on a hardcoded lookup that Defense Grid has a basic value of 4.

I still have no idea why you object to decoupling the "capacity" stat from actual effects. Doing so would make it easier to have a sensible statistic (the scripted capacity) for the AI or interface to use, which represents a typical situation, while letting the effects do whatever is desired without needing to work around an effect generated from the capacity stat that doesn't do what is desired.

[Bigjoe5]

I still have no idea why you object to decoupling the "capacity" stat from actual effects. Doing so would make it easier to have a sensible statistic (the scripted capacity) for the AI or interface to use, which represents a typical situation, while letting the effects do whatever is desired without needing to work around an effect generated from the capacity stat that doesn't do what is desired.

For me, at least, the main concern is duplicate code. Needing to keep different numbers aligned is a maintenance hassle and a barrier to newcomers.

[Dilvish]

I still have no idea why you object to decoupling the "capacity" stat from actual effects.

Really?

I don't see decoupling display of capacity from the thing itself to be a solution, it looks to me like a crude hack that only accomplishes only a small portion of the functionality and adds a different error-prone maintenance issue whose relative plusses and minuses balance out to a very similar error risk.

I had gotten the impression that you understood my concern well enough before. It's true that now I have raised a new one, but do you not feel you understand my interest in having the AI be able to retrieve at least a base value for a part it's considering in a design? Besides trying to minimize code duplication as BigJoe notes, I would like that AI to be able to be at least a little bit adaptive to user-customized parts, which means minimizing the use of values hard-coded into the AI. If the AI only has to rely on a hardcoded rule about shields (other than robotic interface shields) not stacking (which it already has to do anyway), then that's not nearly so bad as also having to rely on hardcoded values.

The way I understood you to be referring to decoupling, it would be useless to the AI. But perhaps you have something else in mind that I'm not understanding. I expect you understand my interests well enough, and I have a hard time believing we need further debate about my understanding of your ideas. If you feel you have a solution here that is better than the one I put up before, then please go ahead and put yours up. I have to admit, I think BigJoe's idea about adding some kind of stackability characteristic to ShipPart specifications is probably the best long term solution for the core problem we are facing here (so that capacity could again be meaningful). But until someone is ready to code up the changes to the parser, or until you come up with some other solution, it seems to me that my solution is a reasonable interim one.

[Geoff the Medio]

For me, at least, the main concern is duplicate code. Needing to keep different numbers aligned is a maintenance hassle and a barrier to newcomers.

These don't carry much weight for me, when the "duplicate code" is a single line with a specific and clear purpose (the UI / AI ranking of the part), and the alternative is a much more complicated design of effectsgroup(s) to work around the (hidden, not obvious) auto-generated effectsgroup.

[Dilvish]

These don't carry much weight for me, when the "duplicate code" is a single line with a specific and clear purpose (the UI / AI ranking of the part), and the alternative is a much more complicated design of effectsgroup(s) to work around the (hidden, not obvious) auto-generated effectsgroup.

Ah, when you had recently referred to "decoupling", I thought you were still referring to

A better / simpler fix might be to just remove the automatically generated strength / capacity text in the descriptions, and add that to them manually, so that custom-scripted situations such as this don't need awkward workarounds to get the right number to show up in the pedia.

The later idea you mentioned and which it sounds like you are focused on now, about having a 'sorting' or 'reference' value be a component of the part specification, sounds like for practical purposes it would work pretty much the same as referencing the capacity, at least for the UI and AI situations I was referring to. It seems to me a little bit of an odd approach, but I'll get over that easily enough if you really want to solve this issue by putting your time into reworking the parser in this fashion. I suppose that if the reference value specifier occurs after the capacity specifier then it could default to the capacity in the event that the reference value is left out of the part script (for weapons and whatnot where the capacity is working just fine).

If there were also a ValueRef that gave the reference value for a named ship part, then the shield and detector Effects we've been discussing could probably be scripted up referring to that and avoiding any code duplication at all (at least in the cases we've discussed so far).

[Geoff the Medio]

Ah, when you had recently referred to "decoupling", I thought you were still referring to

A better / simpler fix might be to just remove the automatically generated strength / capacity text in the descriptions, and add that to them manually, so that custom-scripted situations such as this don't need awkward workarounds to get the right number to show up in the pedia.

... The later idea ... about having a 'sorting' or 'reference' value be a component of the part specification...

They are the same idea: get rid of the automatically generated effects, but still have a simple sorting / UI value. "Decoupling" doesn't mean removing the "capacity" stat, but just removing the autogenerated effects from it.

Decoupling would mean you could tell the UI to use a particular fixed value for sorting, and then use probably-simpler scripts to determine the actual in-game effect of a part.

...if you really want to solve this issue by putting your time into reworking the parser in this fashion.

I've been spending quite a lot of time in the parser code lately, trying to get it to compile and work properly with various changes that should (I think?) work, but it's not cooperating. The hope was the the various change with resolve the other crash issues some were reporting. Small additions like that proposed here are not a big time investment (unless it randomly decides to stop working on select OS builds...)

...a ValueRef that gave the reference value for a named ship part...

Shouldn't be difficult to set up as a ComplexValueRef.

[Geoff the Medio]

...a ValueRef that gave the reference value for a named ship part...

Shouldn't be difficult to set up as a ComplexValueRef.

Haven't tested it, but this should be done in SVN.

[Morlic]

What do you think about the attached approach to the stacking?

The stacking rule used is that only the strongest effect is applied.

Implementation:
Each part has a capacity and an effect which triggers if there are more than two parts of the same class.
The effect simply subtracts the sum of capacities of all used parts and then adds the strongest effect.
The effect is in a stackinggroup and handles all parts of the same class, thus only one tooltip entry is generated.

The relevant macros could be created by a script relatively easily, I suppose. Then all needed to maintain is a single list of part names.

[Dilvish]

What do you think about the attached approach to the stacking?

To me the approach looks like just what the doctor ordered, good job. On visual inspection the more substantive details all look fine; did you test out various combinations? (You can temporarily make the techs and parts cheap/fast to enable fast testing.) One minorish detail is that it seems to me that we shouldn't need an accountinglabel in addition to the stackinggroup (the latter effectively creates its own accountinglabel; my earlier versions of the patch had the accountinglabels since that was a separate EffectsGroup from the stackinggroup one)-- I would just replace the current stackinggroup key with key you have for the accountinglabels, and then drop the accountinglabel line. At least that's what comes to mind, would you have the chance to test that out?

The relevant macros could be created by a script relatively easily, I suppose. Then all needed to maintain is a single list of part names.

The maintenance like this seems fine to me without an extra script, but lets see what opinions others have. The one related thing I would suggest is extra comments at the start of the Detector, Shield and Cloaking groups, that any new parts in these classes need to be added to the respective BEST_X_EFFECT, SUM_X_CAPACITY and STACKED_X_EFFECT macros at the end of the file.

[Morlic]

Thanks for the hint about the accountinglabel. I will try it out and also add some comments to the parts.

I did test some sample of combinations (i.e. the most obvious/important ones). I'll do some more extensive testing after final approval of the approach and when all requested changes and suggestions are added...

[Morlic]

One minorish detail is that it seems to me that we shouldn't need an accountinglabel in addition to the stackinggroup (the latter effectively creates its own accountinglabel; my earlier versions of the patch had the accountinglabels since that was a separate EffectsGroup from the stackinggroup one)-- I would just replace the current stackinggroup key with key you have for the accountinglabels, and then drop the accountinglabel line. At least that's what comes to mind, would you have the chance to test that out?

I Couldn't bring that to work, I added a pull request with accountinglabels.

[Dilvish]

There are part class changes, a bunch of hard-coded and probably difficult-to-maintain macros to implement custom stacking behaviour, and changes to part capacities without doing anything about the C++ hard-coded effect generation that caused some of the issues. I'd probably merge the class change, and nothing else. Discuss the rest more on the forums.

I'll take a first pass at responding to your concerns since I feel that Morlic was largely following my lead. You cite concern about the maintainability of these macros, but I think if you look more at the overall picture you'll see that this improves overall content maintainability. Without this kind of change, there are a number of places in the AI code where we are forced to hardcode these capacity values, so anyone changing these parts would need to also change the AI code, unless these changes or ones like them are made. The current situation is surely a greater maintenance headache than using these macros.

As for the underlying C++ hard coded effect generation, well, I'm not really understanding your concern. if/when BigJoe or whoever gets around to changing that in some way that renders these changes unnecessary, they would have had to adjust these same scripts anyway, and I don't think these changes would make that any more difficult for them. If it helps though, the AI team can commit to adjusting these content scripts again once the modified C++ engine code is rolled out, if the person who does that wants our help with it.

[Geoff the Medio]

Adjusting the C++ to remove automatically-generated C++ effects should be quite easy / fast... Just make PartType::Init and empty function. After that, though, you'd just need to add in FOCS effects for all the parts content that relies on the C++ effects now, and the PartCapacity ValueRef should work fine for that.

A similar adjustment could be done for hulls, to get rid of the C++ effects, but they presently lack a way to get the capacity-equivalent for the autogenerated effects for fuel, stealth, structure, battle speed, and starlane speed stats from the part definition.

Doing those seems to be an independent issue from any stacking modifications, though, so I'd do it as a separate commit (or commits given the extra needs for hulls).

If you think the macros for stacking is preferable to what's done now, then that's also fine... I do think it's a bit difficult to maintain, though, and would prefer a more general solution to stacking control, eventually.

[Morlic]

I think one point you neglect is that use of the macro [[BEST_X_EFFECT]] allows us to choose the strongest part. That is not the case with the current implementation where it is based on the order of effects instead which gives quite unintuitive results (the part first in alphabet works(?)).

Your proposed solution would merely get rid of the [[SUM_X_CAPACITY]] macro. However, considering we have to maintain the [[BEST_X_EFFECT]] macro anyway, I feel like that isn't changing too much (copy+paste 2 lines instead of 1...)
Unless of course you would not use the [[BEST_X_EFFECT]] macro either and prefer the stacking to be inconsistent until some C++ solution is implemented.




I'd futher argue that for any other parts without stacking issues, the current C++ solution is preferable. Writing only capacity = 10 obviously is more elegant than adding another effect.

While I understand that decoupling the capacity value from effects may allow for a cleaner implementation of more complex effects, I think this comes at the price of making simple things overly complex (at least on first sight).
As newcomer without understanding the deeper issues, I'd ask myself - "why would I have to specify this twice - why doesn't the capacity do things on its own? Can I just not use capacity and set the value directly?"
Also, I would wonder why there is the possibility to use one "variable" in FOCS but why I can't specify more variables for different effects.

Is there already some ValueRef to simply refer to the capacity of this part? I feel like that could be a helpful simplification if you really want to decouple the capacity from the effects.

Code: Select all

    effectsgroups =
        EffectsGroup
            scope = Source
            activation = Source
            effects = SetMaxStructure value = Value + PartCapacity name = "NameOfThisPart"

looks kind of ugly for a very basic functionality.