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

Policy: Uploads which are not usable with main/trunk OpenTTD #105

Closed
JGRennison opened this issue Sep 3, 2021 · 6 comments
Closed

Policy: Uploads which are not usable with main/trunk OpenTTD #105

JGRennison opened this issue Sep 3, 2021 · 6 comments

Comments

@JGRennison
Copy link

@JGRennison JGRennison commented Sep 3, 2021

There are uploads on Bananas which don't work with main/trunk OpenTTD, as they require features in non-main versions such as patchpacks. This is a policy query about how these and similar content in future should/shouldn't be addressed.

At the moment these show up in the content list when queried from main/trunk OpenTTD, and can be downloaded, however they don't work when trying to use them.

  • Saves and scenarios will not load at all
  • Scripts (AI/GS) should print an informative message but might just fail or print something cryptic
  • NewGRFs should print an informative message and disable themselves but might just fail or print something cryptic

(Scripts and NewGRFs where any extra functionality is optional and not required should just work without the user needing to be informed. The majority of NewGRFs which use extra features should do this.)

This seems non-ideal for main/trunk users and could result in user support issues.
Paying to host content which your users can't use and which could create extra work could also be seen as a bit problematic.

Some questions:

  • Should such content go on Bananas at all? I had been pondering setting something else up but that is just thoughts at the moment.
  • Is there any suggested tagging, version range or other identifier which should be used?
  • Should any technical measures (e.g. to the TCP content protocol) be taken to remove/hide/omit content which is not compatible with main/trunk OpenTTD?

Some examples of uploads on Bananas:
https://github.com/OpenTTD/BaNaNaS/blob/756b70df03253461991cbe03364629d9e1e02cc1/scenario/000018b2/global.yaml
https://github.com/OpenTTD/BaNaNaS/blob/756b70df03253461991cbe03364629d9e1e02cc1/game-script/454e5054/global.yaml
https://github.com/OpenTTD/BaNaNaS/blob/756b70df03253461991cbe03364629d9e1e02cc1/newgrf/51420102/versions/20201015T123843Z.yaml

@frosch123
Copy link
Member

@frosch123 frosch123 commented Sep 3, 2021

The intention of BaNaNaS 1.5 is to also support forks and patchpacks, called "branches".

Implemented are:

  • The backend REST API supports multiple "branches". Though they have to be defined in the source.
  • The upload clients get the list of branches from the REST API, and thus also support branches.

Not implemented are:

Current user experience from a patchpack user POV: (personal view)
I think most users, who use patchpacks, have multiple versions of OpenTTD installed, and share the download-content between them. So filtering the content on download is not as useful, they would need filtering when adding content to a game.

Current user experience from a vanilla user POV: (personal view)
There is actually various content on BaNaNaS, which does not work for various reasons.

  • Scenarios use NewGRF, which are/were never available on BaNaNaS. Sometimes they use NewGRF from authors, who do not upload stuff to BaNaNaS. Sometimes they use alpha-prereleases from forums/devzone, while BaNaNaS only has the proper release.
  • AIs are infamous to crash all the time.

So, I think, content for forks does not really worsen the current situation.

Summary:

  • Content for forks/patchpacks is welcome on BaNaNaS.
  • The intended method for tagging them ("branches") is not implemented in the download client (OpenTTD).

Maybe we should add "JGRPP" as "branch" to the upload client already, even though the download client won't filter on it?

Loading

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Sep 3, 2021

Paying to host content which your users can't use and which could create extra work could also be seen as a bit problematic.

As @frosch123 said everything I would have said, I just want to add on this snippet.

Personally, and I think the "branches" support shows I am not alone in this, I consider a patchpack as much OpenTTD as vanilla. As such, this really is a non-issue as far as I am concerned. Even if we have to make some changes to the protocol or service, just let me know what you would like to change, and I will make it happen. Well, within reason of course :P
As for the cost .. as long as we distribute both abase and zbase as two different downloads, I am pretty sure nothing else we do will make a dent compared to that :P

tldr: this really is a non-issue as far as I am concerned :)

PS: I absolutely love your post.

Loading

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Sep 3, 2021

beep beep this is your IRC sync bot.

Suggestion:
https://github.com/OpenTTD/OpenTTD/blob/master/src/network/core/tcp_content.h#L33

Extend this packet to contain a list of ("branch", "version") tuple. Example

type: 0
newgrf-version: 0xffffffff (we forgot to add a versioning in the protocol, so we need to tell the server to expect the list variant)
branch: official
version: 12.0
branch: jgrpp
version: 0.42.2

This will give all content that is compatible with "official >= 12.0" or "jgrpp > 0.42.2".
Vanilla will only use "official" ofc.

That would make "branches" in bananas-api useful, and we can add "jgrpp" to it.

This way there is a minimal change to the protocol, enabling this feature.

Loading

@JGRennison
Copy link
Author

@JGRennison JGRennison commented Sep 3, 2021

beep beep this is your IRC sync bot.

Suggestion:
https://github.com/OpenTTD/OpenTTD/blob/master/src/network/core/tcp_content.h#L33

Extend this packet to contain a list of ("branch", "version") tuple. Example

type: 0
newgrf-version: 0xffffffff (we forgot to add a versioning in the protocol, so we need to tell the server to expect the list variant)
branch: official
version: 12.0
branch: jgrpp
version: 0.42.2

This will give all content that is compatible with "official >= 12.0" or "jgrpp > 0.42.2".
Vanilla will only use "official" ofc.

That would make "branches" in bananas-api useful, and we can add "jgrpp" to it.

This way there is a minimal change to the protocol, enabling this feature.

I'd have no objections to this. Sounds like a good change.
Perhaps there should be a "number of branch/version pairs following" value, in case the packet is to be further extended later?

At the moment trailing bytes in CLIENT_INFO_LIST are explicitly checked for here: https://github.com/OpenTTD/bananas-server/blob/main/bananas_server/openttd/receive.py#L83-L84

Loading

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Sep 5, 2021

Right, ran into a small problem: what does it mean if a branch is not mentioned in compatibility. Currently it means: the entry is compatible. And that works fine for most part, except for when you want to say: compatible with official 12.0 or higher. Let me explain:

JGRPP would send: official: 12.0, jgrpp: 0.43.0. So content that says >= 12.0 would match on the first, and all is well. But older clients would send: official: 0.11.2, jgrpp: 0.42.2. Now >= 12.0 doesn't match, but as the content doesn't define jgrpp, it does match. And as such, it is shown again. This is not the intention of the user, of course.

What I suggest we do, is not weight all branches equally. If the official branch is missing from compatibility, it means content is compatible. If any other branch is missing (but the client does send it), it means we are not compatible. Basically, if you say you are not compatible with official, you have to be explicit with what branch you are compatible. The web-interface of course should help the user with this.

Any input from either of you would be greatly appreciated :D

I wrote out some scenarios:

Entry that is compatible with JGRPP and official:

compatibility: []

(not mentioning official branch means you are compatible with it)

Entry that is JGRPP only:

compatibility:
- name: official
  conditions:
  - "< 0.0"
- name: jgrpp
  conditions:
  - ">= 0.0"

(mentioning a branch < 0.0 means you are not compatible with it; but if jgrpp wouldn't be mentioned, it would be skipped. So we need to explicitly write jgrpp too.)

Entry that is JGRPP only, from version 0.42.0:

compatibility:
- name: official
  conditions:
  - "< 0.0"
- name: jgrpp
  conditions:
  - ">= 0.42.0"

(entries are ord, so one of the branches needs to succeed before the client shows it).

Entry is 12.0 compatible:

compatibility:
- name: official
  conditions:
  - ">= 12.0"

(for JGRPP client, official: 0.11.2, jgrpp: 0.42.2 would be a miss on the first, and as jgrpp is not mentioned, also be a miss. So the entry is considered not compatible).

Loading

@frosch123
Copy link
Member

@frosch123 frosch123 commented Sep 5, 2021

I would go for different rules:

  • If compatibility is empty, the content is compatible with everything.
  • Otherwise it is only compatible with the listed branches.

This avoids giving "official" any special meaning.

Applying that to your scenarios: (and some more)

  1. Entry is compatible with JGRPP and official:
    compatibility: []
    
  2. Entry is JGRPP only (but uploader does not know which version exactly):
    compatibility:
    - name: jgrpp
      conditions:
      - ">= 0.0"
    
  3. Entry is JGRPP >= 0.42.0 only:
    compatibility:
    - name: jgrpp
      conditions:
      - ">= 0.42.0"
    
  4. Entry is 12.0 compatible:
    compatibility:
    - name: official
      conditions:
      - ">= 12.0"
    
  5. Entry works in JGRPP >= 0.41, but also in offical >= 13.0. (JGRPP feature merged into official)
    compatibility:
    - name: official
      conditions:
      - ">= 13.0"
    - name: jgrpp
      conditions:
      - ">= 0.41"
    
  6. Entry works in JGRPP >= 0.42 and official >= 12.0. (official features merged/rebases in JGRPP)
    JGRPP sends "official: 12.0", so no special entry needed.
    compatibility:
    - name: official
      conditions:
      - ">= 12.0"
    
  7. Entry works in official 12.0, but has optional special features in JGRPP 0.43.
    compatibility:
    - name: official
      conditions:
      - ">= 12.0"
    
    Adding "JGRPP >= 0.43" has no effect, since everything offical-12.0 is already available to all JGRPP version.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants