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

displayTest option in mods.toml #8656

Merged
merged 2 commits into from
Jun 9, 2022
Merged

displayTest option in mods.toml #8656

merged 2 commits into from
Jun 9, 2022

Conversation

cpw
Copy link
Contributor

@cpw cpw commented Jun 5, 2022

  • "DEFAULT" (or none) is existing match version string behaviour
  • "CLIENTONLY" accepts anything and sends an empty string
  • "SERVERONLY" accepts anything and sends special SERVERONLY string

Signed-off-by: cpw cpw+github@weeksfamily.ca

@Claycorp

This comment was marked as abuse.

@sciwhiz12 sciwhiz12 added Feature This request implements a new feature. Triage This request requires the active attention of the Triage Team. Requires labelling or reviews. 1.18 labels Jun 5, 2022
@cpw
Copy link
Contributor Author

cpw commented Jun 5, 2022

Oh hey look! The logical way to handle this for 99.99% of people finally made it only after it was shot down multiple times for "being the wrong way" and "that's not what the mods.toml file is for". Wasn't so hard after all either!

Gad to see Forge using some common sense on this one, even though it's like 3 years late.

I beg your pardon? Is that an appropriate comment? Is this useful feedback on this PR?

@MelanX
Copy link
Contributor

MelanX commented Jun 5, 2022

I think it’d be good to also edit the MDK in this PR.

@cpw
Copy link
Contributor Author

cpw commented Jun 5, 2022

One thing I want to make sure to caution. This is the display test compatibility flag. It has nothing to do with where the mod can load.

Mods should always be loadable wherever they find themselves. There are a whole host of mechanisms to prevent unwanted code running on the wrong side (server vs client). This is 100% a utility for mods that tend to target one side or another and don't do network comms.

@TheCurle
Copy link
Contributor

TheCurle commented Jun 5, 2022

This PR is awaiting some documentation in the mods.toml file.
I have asked some team members to write this in an internal channel, so it should not be too long.

@Lupicus
Copy link
Contributor

Lupicus commented Jun 6, 2022

What about the case where the mod can be loaded on either side. I have a mod that has both server and client code and no network comms. I guess I could split into 2 mods, but having another option would be good to avoid people thinking a mod is only good for one side.

@gigaherz
Copy link
Contributor

gigaherz commented Jun 6, 2022

The names are a bit misleading:

  • DEFAULT -> both required
  • CLIENTONLY -> the client will show the green checkmark if the server is missing the mod, but the red mark would still show if the client is missing the mod
  • SERVERONLY -> the client will show the green checkmark if the client is missing the mod, but also if the server is missing the mod.

Personally, I'd change the names to:

  • DEFAULT
  • SERVER_OPTIONAL
  • EITHER_OPTIONAL

@Lupicus
Copy link
Contributor

Lupicus commented Jun 6, 2022

I guess if not changed, I could put a comment on that line saying that mod includes client code

@cpw
Copy link
Contributor Author

cpw commented Jun 6, 2022

What about the case where the mod can be loaded on either side. I have a mod that has both server and client code and no network comms. I guess I could split into 2 mods, but having another option would be good to avoid people thinking a mod is only good for one side.

If your mod works whatever version is on the other side, then it's truly two independent mods. But I very much doubt that's the case, and in fact, you want the standard network test, or some proper version checking.

Copy link
Contributor

@marchermans marchermans left a comment

Choose a reason for hiding this comment

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

Principally this is fine, the removal of the extension points if not set is a bit of code smell to me.

But in general this if fine, as long as no mod code could have been run before this is executed (like the mod constructor).

if (displayTestSupplier != null)
registerExtensionPoint(IExtensionPoint.DisplayTest.class, displayTestSupplier);
else
extensionPoints.remove(IExtensionPoint.DisplayTest.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

Personal opinion: Removing the extension point is not a great idea.

But I guess this runs before mod construction so no extension point could have been registered so it should not really matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, it's absolutely fine. Removing it completely is a totally legitimate option, as demonstrated by the lowcode langprovider. It also means that you can replace it with your own, if you wish.

Copy link
Contributor

Choose a reason for hiding this comment

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

But that is my question basically: if there was already an option for MO code to run, is it then not feasible that a modder could already have it set? Or has no mod code been invoked at all at this point?

@cpw cpw changed the base branch from 1.18.x to 1.19.x June 9, 2022 19:58
@cpw cpw added 1.19 and removed 1.18 labels Jun 9, 2022
cpw added 2 commits June 9, 2022 16:19
* "DEFAULT" (or none) is existing match version string behaviour
* "CLIENTONLY" accepts anything and sends an empty string
* "SERVERONLY" accepts anything and sends special SERVERONLY string

Signed-off-by: cpw <cpw+github@weeksfamily.ca>
…in mdk.

Signed-off-by: cpw <cpw+github@weeksfamily.ca>
@cpw cpw merged commit 27480c1 into MinecraftForge:1.19.x Jun 9, 2022
@cpw cpw deleted the displaytest branch June 9, 2022 20:48
@autoforge autoforge bot removed the Triage This request requires the active attention of the Triage Team. Requires labelling or reviews. label Feb 14, 2023
SrRapero720 added a commit to SrRapero720/MinecraftForge that referenced this pull request Nov 6, 2023
LexManos pushed a commit that referenced this pull request Nov 8, 2023
SrRapero720 added a commit to SrRapero720/MinecraftForge that referenced this pull request Nov 12, 2023
SrRapero720 added a commit to SrRapero720/MinecraftForge that referenced this pull request Feb 29, 2024
SrRapero720 added a commit to SrRapero720/MinecraftForge that referenced this pull request Feb 29, 2024
SrRapero720 added a commit to SrRapero720/MinecraftForge that referenced this pull request Mar 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.19 Feature This request implements a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants