Bugs in latest weekly

For what's not in 'Top Priority Game Design'. Post your ideas, visions, suggestions for the game, rules, modifications, etc.

Moderator: Oberlus

Post Reply
Message
Author
User avatar
Grummel7
Space Dragon
Posts: 335
Joined: Mon Oct 09, 2017 3:44 pm

Bugs in latest weekly

#1 Post by Grummel7 »

I've updated again and then I started digging. I haven't done much private programming lately, but I am a programmer and FO now made me install eclipse and dig into it :twisted:

The first problems is obvious and easy to solve:
Good influence should never be bad, but at the moment it creates negative values for small planets, because the upkeep costs applied before GI multiplies the value with 1.5 (home world independence movement is applied before as well).

Either Good Influence should be applied before the costs or it should be limited like to 0 like:

Code: Select all

            effects = SetTargetInfluence value = max(Value*[[GOOD_MULTIPLIER]], Value)

The second did cost some more effort, but I learnt a lot about FO-scripting.
The Regional Administration is not working:

Code: Select all

             (JumpsBetween
                    object = Source.ID
                    object = Statistic Min //...
                        value = LocalCandidate.ID
                        condition = And [
                            Or [Capital Building name = "BLD_REGIONAL_ADMIN"]
                            OwnedBy empire = Source.Owner
                        ]
                 < 5
                )
Well, it should be JumpsBetweenByEmpireSupplyConnections I guess, but when I try using it, my safe file doesn't load properly anymore (I guess the scripting threw an exception, but where can I see it?) It isn't yet implemented anyway.

More important, choosing one object arbitrarily does not work correctly when a planet is connected to the capital and one or more regional admins.

The following looks better:

Code: Select all

        EffectsGroup    // close supply connected to capital planet is more stable
            scope = Source
            activation = And [
                Planet
                Not Unowned
                ResourceSupplyConnected empire = Source.Owner condition = And [
                    Or [Capital Building name = "BLD_REGIONAL_ADMIN"]
                    OwnedBy empire = Source.Owner
                ] // this should ensure that there is a capital / admin centre object owned by this empire for the later Statistic calculation
                Not Capital
                (Statistic Min
                    value = JumpsBetween
                    object = Target.SystemID
                    object = LocalCandidate.SystemID
                    condition = And [
                        Or [Capital Building name = "BLD_REGIONAL_ADMIN"]
                        OwnedBy empire = Source.Owner
                    ]
                 < 5
                )
                
            ]
            accountinglabel = "CAPITAL_CONNECTION_LABEL"
            effects = SetTargetHappiness value = Value + min(5, 5 - Statistic Min
                value = JumpsBetween
                object = Target.SystemID
                object = LocalCandidate.SystemID
                condition = And [
                    Or [Capital Building name = "BLD_REGIONAL_ADMIN"]
                    OwnedBy empire = Source.Owner
                ]
            )

        EffectsGroup    // far supply connected to capital planet is less
            scope = Source
            activation = And [
                Planet
                Not Unowned
                ResourceSupplyConnected empire = Source.Owner condition = And [
                    Or [Capital Building name = "BLD_REGIONAL_ADMIN"]
                    OwnedBy empire = Source.Owner
                ]
                Not Capital
                (Statistic Min
                    value = JumpsBetween
                    object = Target.SystemID
                    object = LocalCandidate.SystemID
                    condition = And [
                        Or [Capital Building name = "BLD_REGIONAL_ADMIN"]
                        OwnedBy empire = Source.Owner
                    ]
                 > 5
                )
                
            ]
            accountinglabel = "CAPITAL_POOR_CONNECTION_LABEL"
            effects = SetTargetHappiness value = Value + max(-5, 5 - Statistic Min
                value = JumpsBetween
                object = Target.SystemID
                object = LocalCandidate.SystemID
                condition = And [
                    Or [Capital Building name = "BLD_REGIONAL_ADMIN"]
                    OwnedBy empire = Source.Owner
                ]
            )
There is however a potential problem with that as well:
JumpsBetween returns -1 for no connection. This should never be the case, unless somebody puts a Regional Administration on the Experimentor Outpost :lol:
When JumpsBetweenByEmpireSupplyConnections is implemented, it better returns INT_MAX, or the scripting will be broken again.
Is there a good reason why these functions return -1?

Also I wonder whether we could assign that distance to a NamedInteger instead of calculating it up to three times.

And whether I should switch to trunk and start pushing suggested fixes via git. :roll:

Regards,
Anvil

User avatar
Geoff the Medio
Programming, Design, Admin
Posts: 13587
Joined: Wed Oct 08, 2003 1:33 am
Location: Munich

Re: Bugs in latest weekly

#2 Post by Geoff the Medio »

Anvil wrote: Mon Apr 05, 2021 5:14 pmGood influence should never be bad, but at the moment it creates negative values for small planets, because the upkeep costs applied before GI multiplies the value with 1.5 (home world independence movement is applied before as well).
Should be resolved by this.
[JumpsBetween] should be JumpsBetweenByEmpireSupplyConnections I guess, but [...] it isn't yet implemented anyway.
It isn't, which is why the simpler JumpsBetween was used for now.
Also I wonder whether we could assign that distance to a NamedInteger instead of calculating it up to three times.
Depends what you mean by "calculating it"...

User avatar
Geoff the Medio
Programming, Design, Admin
Posts: 13587
Joined: Wed Oct 08, 2003 1:33 am
Location: Munich

Re: Bugs in latest weekly

#3 Post by Geoff the Medio »

Anvil wrote: Mon Apr 05, 2021 5:14 pmThere is however a potential problem with that as well:
JumpsBetween returns -1 for no connection. This should never be the case, unless somebody puts a Regional Administration on the Experimentor Outpost :lol:
When JumpsBetweenByEmpireSupplyConnections is implemented, it better returns INT_MAX, or the scripting will be broken again.
Is there a good reason why these functions return -1?
-1 is the standard "error" / "none" / "all" sentinel value.

In some contexts a ternary comparison could be used... Rather than (JumpsBetween < 5) instead write (0 <= JumpsBetween < 5). That should ensure it doesn't match when there is no connection between the objects.

User avatar
Geoff the Medio
Programming, Design, Admin
Posts: 13587
Joined: Wed Oct 08, 2003 1:33 am
Location: Munich

Re: Bugs in latest weekly

#4 Post by Geoff the Medio »

Anvil wrote: Mon Apr 05, 2021 5:14 pm

Code: Select all

SetTargetHappiness value = Value + min(5, 5 - Statistic Min value = JumpsBetween
    object = Target.SystemID
    object = LocalCandidate.SystemID
    condition = And [
        Or [Capital Building name = "BLD_REGIONAL_ADMIN"]
        OwnedBy empire = Source.Owner
    ]
)
Something like that does make more sense, although it would currently also need a nested JumpsBetween test to make sure they are connected so that the minimum isn't just -1, as (I think), you noted...

User avatar
Grummel7
Space Dragon
Posts: 335
Joined: Mon Oct 09, 2017 3:44 pm

Re: Bugs in latest weekly

#5 Post by Grummel7 »

Geoff the Medio wrote: Mon Apr 05, 2021 7:47 pmDepends what you mean by "calculating it"...
Well, it = Number of Jumps to the nearest connected Administration or the Capital.
The script calculates it, to check whether it is <5.
The script calculates it, to check whether it is >5.
If one condition is true, it's calculated once more to determine the actual modification.

That means code duplication and repetition of a non-trivial calculation. :x
Geoff the Medio wrote: Mon Apr 05, 2021 8:01 pm -1 is the standard "error" / "none" / "all" sentinel value.

In some contexts a ternary comparison could be used... Rather than (JumpsBetween < 5) instead write (0 <= JumpsBetween < 5). That should ensure it doesn't match when there is no connection between the objects.
Well, INT_MAX could here be used do the same, with the advantage that it sorts in naturally. Using the ternary expression would not help, because the Statistic Min produces the wrong result, if e.g. an Admininstration has distance 1, but the Capital is not connected (=-1). You'd have to put ResourceSupplyConnected into the condition.

User avatar
Geoff the Medio
Programming, Design, Admin
Posts: 13587
Joined: Wed Oct 08, 2003 1:33 am
Location: Munich

Re: Bugs in latest weekly

#6 Post by Geoff the Medio »

Anvil wrote: Mon Apr 05, 2021 8:54 pm
Geoff the Medio wrote: Mon Apr 05, 2021 7:47 pmDepends what you mean by "calculating it"...
Well, it = Number of Jumps to the nearest connected Administration or the Capital.
The script calculates it, to check whether it is <5.
The script calculates it, to check whether it is >5.
If one condition is true, it's calculated once more to determine the actual modification.

That means code duplication and repetition of a non-trivial calculation. :x
Putting it in a named ref won't help with the repeated evaluation, though it would reduce the code duplication.

User avatar
Grummel7
Space Dragon
Posts: 335
Joined: Mon Oct 09, 2017 3:44 pm

Re: Bugs in latest weekly

#7 Post by Grummel7 »

Geoff the Medio wrote: Mon Apr 05, 2021 8:10 pm Something like that does make more sense, although it would currently also need a nested JumpsBetween test to make sure they are connected so that the minimum isn't just -1, as (I think), you noted...
You mean something like that:

Code: Select all

JUMPS_TO_CAPITAL_OR_REGAD
'''
        Statistic Min
            value = JumpsBetween
            object = Source.ID
            object = LocalCandidate.ID
            condition = And [
                Or [Capital Building name = "BLD_REGIONAL_ADMIN"]
                OwnedBy empire = Source.Owner
                (JumpsBetween object = Source.ID object = LocalCandidate.ID >= 0)
            ]
'''
I would call it a macro, but whatever it is called, it makes the script a lot easier to read and maintain.
Of course in this version, even the nested JumpsBetween does still not check for whether the candidate is connected via supply chain, but when JumpsBetween is replaced by JumpsBetweenByEmpireSupplyConnections, the code should do.

Btw.: while playing to get a better test scenario I encountered the problem that on some planets I could queue in a regional administration, but when finished the turn, the building was simply removed from the build queue.
Any ideas?

User avatar
Grummel7
Space Dragon
Posts: 335
Joined: Mon Oct 09, 2017 3:44 pm

Re: Bugs in latest weekly

#8 Post by Grummel7 »

Actually I found the bug that cause the removal of the regional administration:
The script checks for any capital within 5 star lanes. As a player I could only guess that the Trith capital was close by, my client did not know it, the server did.

The Script should be amended like this:

Code: Select all

BuildingType
    name = "BLD_REGIONAL_ADMIN"
    ...
        Not WithinStarlaneJumps jumps = 5 condition = And [ Or [
	            Building name = "BLD_IMPERIAL_PALACE"
	            Building name = "BLD_REGIONAL_ADMIN"
            ]
            OwnedBy empire = Source.Owner
        ]
    ]
Btw. there is also a little inconsistancy that this script checks for BLD_IMPERIAL_PALACE while happiness checks for Capital. I can actually scrap the Imperial Palace and the planet still counts as having a good capital connection, while I can build a regional admin in a close by system. :lol:

User avatar
Geoff the Medio
Programming, Design, Admin
Posts: 13587
Joined: Wed Oct 08, 2003 1:33 am
Location: Munich

Re: Bugs in latest weekly

#9 Post by Geoff the Medio »

Will you make some github pull requests for such fixes?

That asked, I wonder if there's a potential multiplayer exploit if players trade planets in order to produce region admins on them despite the (temporarily) other empire's nearby capital or admin buildings. Might need to remove such buildings in the event another empire owns the planet than the one that produced the building...

User avatar
Grummel7
Space Dragon
Posts: 335
Joined: Mon Oct 09, 2017 3:44 pm

Re: Bugs in latest weekly

#10 Post by Grummel7 »

Geoff the Medio wrote: Thu Apr 08, 2021 9:54 pm Will you make some github pull requests for such fixes?
Once I've figured out how to do that, I'll gladly do.

I have recovered my year old GitHub Account (that I have only created because some organisation wanted an authorisation token), set up a personal access token and still cannot even setup the git-hub command line interface. I also received an emails saying soon I need to use MFA.

~/freeorion-project/freeorion$ git-hub setup
Using Git repository local configuration
GitHub username [xxx]: Grummel7
GitHub password (will not be stored):
Looking for GitHub authorization token...
Error: GitHub error: Not Found

And I thought all I need is git... :roll:

User avatar
Geoff the Medio
Programming, Design, Admin
Posts: 13587
Joined: Wed Oct 08, 2003 1:33 am
Location: Munich

Re: Bugs in latest weekly

#11 Post by Geoff the Medio »

Anvil wrote: Sat Apr 10, 2021 6:03 pmAnd I thought all I need is git... :roll:
You don't need any GitHub specific software on your own machine to fetch / pull / rebase / push etc. You do obviously need an account on github, but the pushing and such just uses git as far as I'm aware.

User avatar
Grummel7
Space Dragon
Posts: 335
Joined: Mon Oct 09, 2017 3:44 pm

Re: Bugs in latest weekly

#12 Post by Grummel7 »

I did it :D , well, almost... :?
Part of my problem was that since the account was old, GitHub did not show me the new users help.

Anyway, now I have my fork of freeorion (that part I had to Google, I have no idea where to find it in the GitHub documentation), submitted my two changes to it and created a pull request of the first.

I wanted to create a pull request for the second, but when I try, it always includes the first commit as well. I am to tired to keep searching for today. Do I miss something obvious?

User avatar
Geoff the Medio
Programming, Design, Admin
Posts: 13587
Joined: Wed Oct 08, 2003 1:33 am
Location: Munich

Re: Bugs in latest weekly

#13 Post by Geoff the Medio »

A pull request is "request" to merge a branch into master. If you have two separate commits you want to make separate pull requests for, you need two separate branches, both based off master, each with one of those commits in it and not the other.

Post Reply