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

upgrade working for 5.1 #1051

Closed
wants to merge 4 commits into from
Closed

upgrade working for 5.1 #1051

wants to merge 4 commits into from

Conversation

tsky1971
Copy link

@tsky1971 tsky1971 commented Mar 4, 2024

fix iwyu (https://docs.unrealengine.com/5.0/en-US/include-what-you-use-iwyu-for-unreal-engine-programming/)
fix/add missing category for blueprint (needs for 5.1)
compile with 5.1
add build batch (buildpath is more temp - long name of the plugin leads easy to max path length >254)

@gallonmate
Copy link
Contributor

Thanks for adding improvements/changes. The plugin is currently compatible with all versions UE5.0 to 5.3, using Windows and Visual Studio. I'm adding comments/reviews to remove references to UE5.1, which break compatibility with UE5.0.

Also it would be good to update the Unreal plugin Readme so that people are aware of the buildplugin.bat and how to use it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Keep reference to 5.0

Copy link
Contributor

Choose a reason for hiding this comment

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

Keep reference to 5.0

Copy link
Contributor

Choose a reason for hiding this comment

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

Keep reference to 5.0

Copy link
Contributor

Choose a reason for hiding this comment

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

If wanting to update plugin version number, I'd suggest version 1.02

        "Version": 102,
	"VersionName": "1.02",

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to keep no engine version reference if possible, or at least use 5.0 as minimum (if possible).
Also update the Unreal Readme to make note this new bat file exists 😃 and it's usage.

Copy link
Contributor

Choose a reason for hiding this comment

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

These are automatic changes from UE5, no need to include changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

These are automatic changes from UE5, no need to include changes.

fix/add missing category for blueprint
compile with 5.1
add build batch
fix/add missing category for blueprint
compile with 5.1
add build batch
Copy link

codecov bot commented Apr 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 24.97%. Comparing base (3c7582e) to head (b9a3eeb).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1051   +/-   ##
=======================================
  Coverage   24.97%   24.97%           
=======================================
  Files         168      168           
  Lines       18057    18057           
=======================================
  Hits         4510     4510           
  Misses      13547    13547           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bcoconni
Copy link
Member

@gallonmate, @tsky1971 is this PR ready for merging ? Or are there still some pending issues ?

@gallonmate
Copy link
Contributor

gallonmate commented Apr 29, 2024

@gallonmate, @tsky1971 is this PR ready for merging ? Or are there still some pending issues ?

No this PR is not ready. I'm not even 100% sure this PR is necessary in the first place BUT I saw no harm as long as the author resolved the compatibility for all UE5 versions, which they did not address yet, (This PR breaks compatibility with UE5.0). If you don't hear anything from the author, feel free to close the PR.

@lgzajac
Copy link

lgzajac commented May 15, 2024

My 2 cents here. First, thanks for the great work.

Second.

This PR breaks compatibility with UE5.0

There were major changes between 5.0 and 5.1 as I read... With some very naughty fixes partially based on this PR I managed to compile and run jsbsim on 5.3.2 linux (popos). (Half time startup crashes, but i need to get more acquainted with unreal - it is a new stuff for me ;) )

So.. If it possible please do not remove this PR. As unfinished as it is, it is still useful for a newcomers like me :)

Bests.

@gallonmate
Copy link
Contributor

@lgzajac When closing a PR, it's just marked as closed. It can still be found when searched via keywords. The PR is not actually removed. Also there is no major changes between UE5.1 and UE5.0 that required breaking the plugin's compatibility. Keeping compatibility is easy but the author never finished addressing the reviews/comments from 2 months ago. Closing the PR for now allows the JSBSim repo maintainers to keep clarity and focus. UE5.1 was released in Nov 2022 and there are been enough users without complaint, including the original Epic employee who created the plugin, that I do no see any concern. Although I tested the new UE5.4 and it might actually need small fixes similar to this PR. Since the author is busy/missing then closing this PR and creating a new PR is the most likely outcome.

@bcoconni
Copy link
Member

Closing this PR following @gallonmate's suggestion.

As a side note, if the information contained in this pull request is valuable to the community, it may be beneficial to contribute a new article to our wiki rather than expecting non-developers to delve into the PRs.

@bcoconni bcoconni closed this May 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants