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

Breaking Change Meta Issue #747

Open
45 tasks
atteneder opened this issue Jan 7, 2025 · 1 comment
Open
45 tasks

Breaking Change Meta Issue #747

atteneder opened this issue Jan 7, 2025 · 1 comment
Labels
enhancement New feature or request refactor Code base improvements, not necessarily feature/API changes.
Milestone

Comments

@atteneder
Copy link
Owner

atteneder commented Jan 7, 2025

This is a collection of API or behavior breaking changes that are subject to be addressed in the next major release.

Preparations

  • Add Async suffix to all async methods (gh issue) and deprecate the old names.
  • Go through accepted SonarQube issues and evaluate if they can be resolved or require breaking changes.

7.0

Maintenance

  • Bump minimum required version to 2021 LTS (Enterprise EOL mid-October 2025) or 2022 LTS (Enterprise EOL mid-April 2026)
  • Update all dependencies and soft dependency versions
  • Drop support for com.unity.rendering.hybrid and Entities versions 0.x
  • Remove
    • Everything tagged with Obsolete attribute
    • Schema.MeshPrimitiveBase: GetHashCode,Equals
    • Schema.Attributes: GetHashCode,Equals
    • Schema.MorphTarget: GetHashCode,Equals
    • Schema.TextureBase: GetHashCode,Equals
  • Remove Legacy Shader Graphs (if feasible).
  • Consistent nodeId type uint in GltfWriter
    • AddMeshToNode
  • Remove interfaces' default implementations that were added just to not break the API.
    • ICodeLogger.Log

Behavioral

  • Change default logger to be ConsoleLogger without taking away the opportunity to unset the logger to null.
  • Finish Builder construction for GltfImport?
  • Consider making all asmdefs auto-referenced to avoid users not getting how to setup manual references in their asmdefs.

Code Style and Refactorings

  • Seal all classes not intended for inheritance.
    • GltfEntityAsset ?
    • GltfImport ? potentially after refactoring it.
  • IInstantiator.AddPrimitive
    • Rename IInstantiator.AddPrimitive to IInstantiator.AddMesh(Result). Not (glTF) primitives are added/assigned, but MeshResults.
    • AddPrimitive parameter morphTargetWeights can arguably be part of MeshResult.
    • Remove MeshResult.materialIndices. Users should instead use MeshResult.primitiveIndices to query the material from the MeshPrimitive.
    • If you then also remove the meshIndex from MeshResult (and re-add it as AddPrimitive paramter), it becomes identical to MeshAssignment. Consequently MeshAssignment could be replaced with MeshResult.
  • (TBD) Remove GltfImport.GetSource* and instead offer RootBase GltfImport.GetSourceRoot and add all kinds of query methods to RootBase. E.g.: RootBase.GetMesh(index).
  • IInstantiator.CreateNode: Add node name as parameter instead of IInstantiator.SetNodeName.
  • Don't hard-code node name fallback to mesh primitive name in GltfImport. Implement it in (GameObject)Instantiator instead.
  • GltfImport.InstantiateScene should return a generic SceneInstance type (GameObjectSceneInstance or EntitySceneInstance).
  • Use NativeArray<T>.ReadOnly wherever it's not required or dangerous to change the data.

Schema Classes

  • Settle JsonUtility/Newtonsoft discrepancy
  • Ensure metadata round-trip
    • Finish arbitrary access (via Newtonsoft or similar)
  • Add schema classes required for Animation Pointer (iirc animation clip extensions?)
  • If Newtonsoft stays, add extension classes to everything, so that adding it later on won't introduce API breaks.

Vague Technical Debt (to be triaged)

  • Remove as many scripting defines as possible.
    • Investigate if they can be turned into runtime settings.
    • Remove package/version define dependant API alternations.
    • Evaluate if GLTFAST_THREADS is still required (for Web builds).
  • Go through all TODO, FIXME, HACK comments
  • Go through all Unity version scripting defines and remove if possible
    • #if UNITY_202x_y_OR_NEWER
  • Go through Monobehavior methods (Awake,Start,...) that are async void and see if they can be turned to async Task (removes SonarQube complaint).
@atteneder atteneder added enhancement New feature or request refactor Code base improvements, not necessarily feature/API changes. labels Jan 7, 2025
@atteneder atteneder added this to the 7.0 milestone Jan 7, 2025
@atteneder atteneder moved this to Planned in glTFast development Jan 9, 2025
@RichardWepnerPD
Copy link

One remark regarding the very last point:

Go through Monobehavior methods (Awake,Start,...) that are async void and see if they can be turned to async Task (removes SonarQube complaint).

Turning any Monobehaviour methods (those called by Unity) from async void to async Task would have the potential for exceptions not to be logged as expected. There exists a thread on Unity Discussions: https://discussions.unity.com/t/async-and-uncaught-exceptions/824272

If async void really needs to be avoided and void methods should be used instead, something like the following steps would be possible:

  • move all the awaited code into a seperate async Task method (if it's more than a single call)
  • call the (now) single method that returns a Task (previously awaited in the Monobehaviour method) and store the result in a local variable
    • if subsequent calls are possible without the previous one being guaranteed to finish, it needs to be a List<Task>
  • adjust the Update method to check all tasks that are stored and didn't finish yet if their execution finished
    • if it failed or was cancelled, create a log entry
  • remove the Task again (setting to null or removing from a list of tasks) to ensure exceptions are not logged multiple times

This could possibly also be generalized using a separate Component (very fragile, since it or its GameObject could be destroyed by the user), a UnityEngine.PlayerLoop (player loops can technically be removed, however there would need to be more intention to achieve this), a separte Thread with an infinite loop (not very clean), or a timer (would run independent from the unity player loop, so probably also not as clean).

Another alternative would be to investigate if it's possible to add specific exceptions for this case in particular, however this would require an active decision that it's fine to ignore warnings about this specific case. In my opinion, an exception to the rule "async" methods should not return "void" would be justified here, but I'm not the one to make this decision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request refactor Code base improvements, not necessarily feature/API changes.
Projects
Status: Planned
Development

No branches or pull requests

2 participants