Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GS methods to let company have exclusive access to the industry #8115

Merged
merged 1 commit into from Dec 22, 2020

Conversation

ldpl
Copy link
Contributor

@ldpl ldpl commented May 5, 2020

This is now build on top of #7912, which is included in the commits. Merge #7912 before this!


This allows GS to set/reset some company as an exclusive consumer or supplier for the industry. Meaning only that company would be able to take from / deliver to this industry.

This will allow multiplayer servers to cover most of the industry sharing scenarios that atm have to be enforced by rules and moderation. For example:

  • Make the company "own" funded industry. (set both consumer/supplier to the founder company).
  • Only allow sharing on primary industries (set exclusive consumer to the first company delivering cargo to the non-primary industry).
  • On competitive servers stop companies from helping each other by delivering cargo to the secondary industry (set the first company as exclusive supplier).

I also decided to remove the unused owner field in the industry structure as it's somewhat related.

GS to test stuff (sets both consumer/supplier to the first company delivering or taking cargo):
ownit.zip

It can probably be better to somehow merge this with #7912 to save on network commands and savegame versions but I've no idea what's the proper way to do that.

@nielsmh
Copy link
Contributor

nielsmh commented May 5, 2020

I don't think we'll run out of savegame versions in the foreseeable future (currently at version 218 out of 65535 max), but saving network command id's is definitely something #7912 and this should do. Merge one of the PRs first, then rework the other to use the same command, perhaps?

@nielsmh nielsmh added savegame upgrade component: AI/Game script (squirrel) This issue is related to Squirrel (Scripting language) labels May 5, 2020
@James103
Copy link
Contributor

James103 commented May 5, 2020

I don't think we'll run out of savegame versions in the foreseeable future (currently at version 218 out of 65535 max)

This is not taking into account that patch packs have already allocated some of the version numbers. How will we distinguish save games made with official OpenTTD versions (1.11 and up) from save games made with various patch packs (Spring 2013, ChillPP, JokerPP), given the fact that we'll start to overlap with Spring 2013 in just 2 savegame versions from now?

@ldpl ldpl force-pushed the industry-sharing branch 2 times, most recently from e53378a to 823a43b Compare July 14, 2020 08:37
@TrueBrain TrueBrain added candidate: yes This Pull Request is a candidate for being merged size: large This Pull Request is large in size; review will take a while labels Dec 14, 2020
@@ -3222,7 +3222,7 @@ static void GetTileDesc_Station(TileIndex tile, TileDesc *td)
case STATION_OILRIG: {
const Industry *i = Station::GetByTile(tile)->industry;
const IndustrySpec *is = GetIndustrySpec(i->type);
td->owner[0] = i->owner;
td->owner[0] = OWNER_NONE;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also decided to remove the unused owner field in the industry structure as it's somewhat related.

Do I understand you correctly that it has no effect on the PR if you would not do this? As when reading the patch I was confused by this change, and I do not really see why/how this belongs here?
If I understand you correct, and you are just cleaning up unused stuff, I would prefer this being in its own commit at least, but honestly .. if that is the case, I would not do it at all in this PR. A new PR to clean up these things would be better, as that means this change doesn't distract from the feature itself :)

@TrueBrain
Copy link
Member

This is currently missing a GUI indication to tell an industry is exclusively assigned to a player, right? (checking to see if my patch-reading-skills are correct :D). Would you consider that a problem during gameplay?

@TrueBrain TrueBrain added the work: minor details This Pull Request has some minor details left to do label Dec 22, 2020
@ldpl
Copy link
Contributor Author

ldpl commented Dec 22, 2020

Yeah, GUI is missing but that's somewhat intentional as at this point I don't fully know how ppl are going to use this api and what would be the most appropriate thing to do with gui. Also default ui can only show what is happening and can't possibly explain why as only GS itself knows that and imo that's way more important information. So it would be much better to let GS explain things as it see fits and add methods for it to do so if existing aren't enough. For example, I plan to use this thing to limit competition on secondary industries to "first come - first served". Default UI can only show smth like "exclusive to x" but that is almost useless information. Instead I can just write in the story book that gs prevents competition on secondary industries.

So, basically, I think it's ok to add this as it is and do a gui later if it's needed when we have a more clear understanding of what it should show and how.

@TrueBrain TrueBrain removed savegame upgrade work: minor details This Pull Request has some minor details left to do labels Dec 22, 2020
LordAro
LordAro previously approved these changes Dec 22, 2020
Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code looks fine, except for the probably unnecessary saveload block mentioned in the other PR

Presume you've tested suitably!

@TrueBrain TrueBrain merged commit 9a45a0f into OpenTTD:master Dec 22, 2020
@ldpl ldpl deleted the industry-sharing branch September 19, 2021 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: yes This Pull Request is a candidate for being merged component: AI/Game script (squirrel) This issue is related to Squirrel (Scripting language) size: large This Pull Request is large in size; review will take a while
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants