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.

Sign up for GitHub

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

Jump to bottom

Asset store update #634

Closed
wants to merge 2 commits into from
Closed

Asset store update #634

wants to merge 2 commits into from

Conversation

steinbitglis
Copy link
Contributor

Hey Antoine!
This is not a pull request that I actually want you to merge anywhere,
but I'm in need of a little bit of code review. I've held back on updating
the Unity Asset Store version of YamlDotNet, because C# 8 features were being used
after version 6.1.2. But now that Unity has a much upgraded compiler that
supports C# 8, I was making another attempt, just to stay on par with your
progress. Unity now supports .net standard 2.0. There were a few
things in YamlDotNet 11.2.1 that required .net standard 2.1, but I think it's
pretty close.

So what this is, is just me asking if you could review my branch unity_compatible,
and see if you think I broke anything. I don't have a proper IDE set up for
the project, and I'm not able to run the test suite. It would be a really big job
for me just to run the tests, but if it's easier for you, could you do a quick review
and maybe a sanity check (if I've misunderstood the .net standard 2.1 features
or something.). The sample code seems to run alright in Unity, but I haven't
checked the output properly.

If you think it looks ok, I'll go ahead and package it up for the asset store, and
probably also figure out which exact Unity version is recent enough for this updated
code.

@aaubry
Copy link
Owner

aaubry commented Sep 16, 2021 via email

@aaubry
Copy link
Owner

aaubry commented Sep 17, 2021

I see that you have removed a bunch of nullable annotations, such as [MaybeNullWhen(true)]. Those are needed for other platforms, so I would like to avoid removing them. Can you explain how you are compiling this with Unity so that I can try a fix that works for all cases?

@steinbitglis
Copy link
Contributor Author

Yes, I've exported a unitypackage that you can look at.
YamlDotNet_as_unitypackage.zip

Unzip, then import the package from "Assets"->"Import Package"->"Custom Package".
There is a file called YamlDotNet.asmdef, this makes unity compile the folder and everything beneath it into one assembly.

Create a new game object in a scene, and put an ExampleRunner on it. Press play.

The newer versions of Unity will be compiling to .net standard 2.0

@steinbitglis
Copy link
Contributor Author

Did you manage to run the examples in Unity? Let me know if I can help further.

@aaubry
Copy link
Owner

aaubry commented Sep 28, 2021

Did you manage to run the examples in Unity? Let me know if I can help further.

I'm trying right now. I'll let you know how it goes.

@aaubry aaubry mentioned this pull request Sep 28, 2021
Open
@aaubry
Copy link
Owner

aaubry commented Sep 28, 2021

So, I followed your instructions and was able to build a project with your package.
Then, I replaced the code from your package and could see the compilation problems, which I was able to fix. I have already pushed this commit that contains the changes needed to compile the code.

Then, I created a new, empty Unity project in the YamlDotNet directory and added the YamlDotNet code by creating symlinks to include the necessary files and directories. This way, I was able to have a working Unity project that uses the code directly from the main repository. The advantage of this is that anyone can check for Unity copatibility issues while working on the project.

This also seems useful as a step to produce the unity package, although I don't know how that step is performed. I have opened a pull request so that you can check this and maybe make any changes needed to create the unity package from there.

Please have a look and let me know if this is useful. Keep in mind that, if you're on Windows, you will need to enable symlinks on Git, by setting core.symlinks to true, as explained here.

@aaubry aaubry mentioned this pull request Apr 29, 2022
Closed
@aaubry aaubry force-pushed the master branch 8 times, most recently from a0f8359 to 78b1ab3 Compare December 2, 2022 22:39
@EdwardCooke
Copy link
Collaborator

This PR appears to be abandoned, it's over a year old. I'm going to close it. There is a couple feature requests for Unity related packages/processes which I'm planning on looking in to in the near future.

@steinbitglis steinbitglis deleted the unity_compatible branch January 13, 2023 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@steinbitglis @aaubry @EdwardCooke