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

[v2] (RFC) Csharp netcore generator supports UnityWebRequest library #14870

Merged
merged 12 commits into from
Mar 10, 2023
Merged

[v2] (RFC) Csharp netcore generator supports UnityWebRequest library #14870

merged 12 commits into from
Mar 10, 2023

Conversation

iBicha
Copy link
Contributor

@iBicha iBicha commented Mar 2, 2023

This is a follow up to #13890, Closes #853

#13890 was a great start (thank you for the work you've done!), but it sits in a private fork so was harder to work with

On top of it, a few changes has been made:

  • Remove irrelevant files to Unity (e.g. like project and solution files, these are Generated by Unity)
  • Added assembly definition files
  • Use NUnit (what Unity uses for tests)
  • Updated the readme with relevant information for Unity

I would love to see this more thoroughly tested, and validate it works well with different spec files, but from testing so far it seems good enough!

There's probably edge cases to deal with (IL2CPP, console support, etc), but perfect is the enemy of good! I hope we can have a foot in the door with this version and incrementally improve.

Also, I can't believe it's been almost 5 years since #853 !

Tagging the technical committee as per contribution guidelines
@mandrean @frankyjuang @shibayan @Blackclaws @lucamazzanti

And @justonia @wing328 for visibility

@wing328
Copy link
Member

wing328 commented Mar 3, 2023

@iBicha thanks for the PR.

I've pushed ab9ad3f to update the samples and pull the latest master which has fixed some build issues in the master branch.

@iBicha iBicha requested a review from wing328 March 3, 2023 15:55
Copy link

@justonia justonia left a comment

Choose a reason for hiding this comment

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

@iBicha @wing328 I was consumed with console ports and wasn't able to put any time into my draft PR, so thanks for putting effort into getting a real PR in-place.

re: There's probably edge cases to deal with (IL2CPP, console support, etc), but perfect is the enemy of good! I hope we can have a foot in the door with this version and incrementally improve.

Our game is live and this code is proven to work on the following platforms:

  • Nintendo Switch (IL2CPP)
  • PS4 + PS5 (IL2CPP)
  • Xbox Series X|S (IL2CPP)
  • iOS, tvOS, macOS (IL2CPP)
  • Windows (IL2CPP and Mono)
  • Steamdeck (IL2CPP via Windows emulation)
  • Linux (IL2CPP and Mono)

The platforms that are untested but I wouldn't expect issues with:

  • Android
  • WebGL
  • Xbox One

Conveniently, no platform specific modifications were required to support the consoles. 👍

@iBicha iBicha requested review from justonia and wing328 and removed request for wing328 and justonia March 4, 2023 15:54
@iBicha iBicha requested a review from wing328 March 4, 2023 15:55
@iBicha
Copy link
Contributor Author

iBicha commented Mar 4, 2023

Added a couple of fixes to support webgl
Tested WebGL and Android IL2CPP

@iBicha
Copy link
Contributor Author

iBicha commented Mar 7, 2023

@wing328 how can we push this PR forward?

@wing328
Copy link
Member

wing328 commented Mar 9, 2023

@iBicha is there anyway to test this locally to ensure the output (auto-generated API client) compiles? I'm no expert in unity. If there's any command or instruction, please share with me.

Ideally we want to setup the tests in the CI (e.g. github workflow) but that's not day 1 requirement.

@iBicha
Copy link
Contributor Author

iBicha commented Mar 9, 2023

@iBicha is there anyway to test this locally to ensure the output (auto-generated API client) compiles? I'm no expert in unity. If there's any command or instruction, please share with me.

Ideally we want to setup the tests in the CI (e.g. github workflow) but that's not day 1 requirement.

I added a Unity specific sample (it made sense)

For manually testing (assuming you don't any anything setup)

  1. Download Unity Hub (which is a manager for Unity versions, and a launcher)
  2. In the Hub, Download a Unity Editor (choose a version, e.g. 2021)
    Screen Shot 2023-03-09 at 10 39 08 AM
  3. Create a new project
    Screen Shot 2023-03-09 at 10 40 14 AM
  4. Into your project files, you should see an Assets folder. Import your generated client (for example the OpenAPIClient-unityWebRequest sample). Drag and drop should work
    Screen Shot 2023-03-09 at 10 43 34 AM
  5. From the top menu, Choose Assets -> Create -> C# Script. Name it PetStoreTest
  6. From the top menu, Choose GameObject -> Create Empty. Name it PetStoreTest
  7. Drag and drop the script you created into the game object.
  8. Double click the script (PetStoreTest.cs) to open it in a code editor, and set its content to the following
using System.Collections.Generic;
using Org.OpenAPITools.Api;
using UnityEngine;

public class PetStoreTest : MonoBehaviour
{
    async void Start()
    {
        var api = new PetApi();
        var list = await api.FindPetsByStatusAsync(new List<string> { "" });
        foreach (var pet in list)
        {
            Debug.Log(pet);
        }
    }
}
  1. Finally, go back to Unity and press the play button. You should see pets logged in the console

Screen Shot 2023-03-09 at 10 51 31 AM


These are the very basic steps to test manually. Additional work can be done to improve with automation:

@wing328
Copy link
Member

wing328 commented Mar 10, 2023

thanks for sharing the details in using and testing the auto-generated client.

Tested WebGL and Android IL2CPP

given that you've already tested it locally, I think we can go ahead to merge this so that more users can try it out and provide feedback.

@wing328
Copy link
Member

wing328 commented Mar 10, 2023

about the CI failure, https://github.com/OpenAPITools/openapi-generator/actions/runs/4376814010/jobs/7670317597

I'll fix that after merging this PR.

@wing328 wing328 merged commit f3960b2 into OpenAPITools:master Mar 10, 2023
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.

[C#] Codegen for Unity
4 participants