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

Update EngineVersion #19

Closed
wants to merge 3 commits into from
Closed

Update EngineVersion #19

wants to merge 3 commits into from

Conversation

ViktorWii
Copy link
Contributor

@ViktorWii ViktorWii commented Jan 31, 2024

Engine version string in Plugins/TolgeeSDK/Tolgee.uplugin could not be parsed ("5.3")

Sorry for bothering you, but here is another minor proposition
If you look at FEngineVersion::Parse, then you would notice that EngineVersion field supposed to have Major.Minor.Patch version

Currently it's just a warning, but still thought that you might agree on this change
(Also double checked that in 4.26 FEngineVersion::Parse works exactly the same and expects Major.Minor.Patch format)

@ViktorWii
Copy link
Contributor Author

P.S. since my team is going to use tolgee as translation platform, would you be interested in some code review and other improvement suggestions ._.

@ViktorWii
Copy link
Contributor Author

ViktorWii commented Jan 31, 2024

Another suggestion is to remove this field whatsoever so that plugin won't be tied to any version

Or set EngineVersion to lowest version available (i.e. 4.26.0), because if you apply my fix, then plugin won't be compatible with any versions lower then 5.3.0

@JanCizmar JanCizmar requested a review from pasotee January 31, 2024 14:00
@ViktorWii
Copy link
Contributor Author

Updated version to 4.26.0

@ViktorWii
Copy link
Contributor Author

ViktorWii commented Jan 31, 2024

Well, I've looked closely at FPluginManager::IsPluginCompatible and it actually will work only for same minor version, so my best suggestion is to simply remove EngineVersion from uplugin

@pasotee
Copy link
Collaborator

pasotee commented Jan 31, 2024

Hey @ViktorWii any review and suggestions are welcome. If you have any, I am happy to take a look and discuss them with you and your team.

Regarding this change specifically, the EngineVersion field is required to be present as per the Unreal Engine Marketplace guidelines - 1.4.2.c.

I fully agree with you, this field should be optional and should be removed from all GitHub plugins. I've been advocating for it to the Marketplace submission, but unfortunately, as long as they require it to be present, having it here makes our lives easier when we submit updates to the Marketplace.

@ViktorWii
Copy link
Contributor Author

Oh, now I see your reasoning...
So do you update it manually each time you publish an update for marketplace?
And do you update it for each EngineVersion? Or it's just set to invalid value to satisfy marketplace requirements?

@pasotee
Copy link
Collaborator

pasotee commented Feb 1, 2024

So do you update it manually each time you publish an update for marketplace?

Unfortunately, yes. There is no CLI, no rest api endpoint, it's just a plain old upload zip. I hope it will change when Epic migrates to fab.com, but currently, it's fully manual.

And do you update it for each EngineVersion? Or it's just set to invalid value to satisfy marketplace requirements?

It depends on the importance of the update. To save time, I usually just update the current engine version (in this case 5.3). If it's something important, I will duplicate the zip, change the engine version, and upload it to Google Drive/WeTransfer.

To give you some more insights, currently, after we create a tag, a release will be created with the source code. Because this matches the current engine version (5.3) I can directly send the public GitHub artifact link to Epic.

Of course, we could run a matrix action on GitHub Actions create 3 zips (one for each version) by injecting the EngineVersion via string/json manipulation. However in my experience that comes with a lot of headaches and the risk vs reward ratio is not enough.

@ViktorWii
Copy link
Contributor Author

@pasotee
I guess you can close this PR. Seems like invalid version and simple warning about it is the best way to go around it

@ViktorWii ViktorWii closed this Feb 1, 2024
@ViktorWii
Copy link
Contributor Author

Couldn't be fixed due to Marketplace requirements

@ViktorWii ViktorWii deleted the patch-1 branch February 1, 2024 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants