Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Expose serialization api#2336

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

BadSingleton
Copy link
Contributor

@BadSingletonBadSingleton commentedMar 7, 2024•
edited
Loading

Adds post-serialization and pre-deserialization hooks for additional customization.

What does this implement/fix? Explain your changes.

This PR supersedes#1892 , offer another workaround for issues#2335#2221#2282 , possibly others.

Does this close any currently open issues?

This addresses@lostmsu 's comments of exposing an API instead of adding functionality

Any other comments?

According to Unity dev answers on the Unity forums, they will implement a ring-link architecture as a replacement for domain load/unload. I have added no tests yet in this PR considering that domain load/unload is deprecated from a .NEt perspective.
https://forum.unity.com/threads/unity-future-net-development-status.1092205/page-34#post-8918231

This PR is made in my own name, I am no longer working at the company that signed the CLA for me.

Checklist

Check all those that are applicable and complete.

  • Make sure to include one or more tests for your change
  • If an enhancement PR, please create docs and at best an example
  • Ensure you have signed the.NET Foundation CLA
  • Add yourself toAUTHORS
  • Updated theCHANGELOG

Adds post-serialization and pre-deserialization hooks for additionalcustomization.
* And revert the breaking change of the FormatterType removal* Add CHANGELOG entry
@BadSingletonBadSingleton mentioned this pull requestMar 7, 2024
5 tasks
@filmor
Copy link
Member

@lostmsu any notes?

@lostmsu
Copy link
Member

Add some tests.

BadSingleton reacted with thumbs up emoji

@lostmsu
Copy link
Member

lostmsu commentedMar 29, 2024•
edited
Loading

BTW, what's your interest in this functionality working?

Cause if there's none, I think we should consider just bumping version to 4.x and dropping domain reload support altogether.

Do you know anyone in Unity who stills works on it? There's been no communication for a while.

@BadSingleton
Copy link
ContributorAuthor

BadSingleton commentedMar 29, 2024•
edited
Loading

BTW, what's your interest in this functionality working

Superseding/closing the previous PR. At this point, considering Unity's public plans, it's not unreasonable to drop this. Unity has their own fork with what was in the previous PR anyway.

Do you know anyone in Unity who stills works on it?

I think so. Last I knew of, another team had assumed maintaintership of the package. I'll be sending a few messages see if they are still at Unity/on those projects

@filmor
Copy link
Member

filmor commentedApr 26, 2024•
edited
Loading

@lostmsu Any more comments? I took the liberty to implement your comments on the formatter factory.

P.S.: The current build failures are due to macos-14, I'll adjust the CI in another PR.

@BadSingleton
Copy link
ContributorAuthor

I'll be sending a few messages see if they are still at Unity/on those projects

To close the loop on this, the people I could get in touch with are no longer at Unity.

/// <param name="stream">A MemoryStream that contains the data to be placed in the capsule</param>
public static void StashSerializationData(string key, MemoryStream stream)
{
var data = stream.GetBuffer();
Copy link
Member

Choose a reason for hiding this comment

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

This might have extra data in the end

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

What do you mean by extra data? The stream container is longer than the data in it? The code is usingLength notCapacity, so it should not be an issue, no?

Copy link
Member

Choose a reason for hiding this comment

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

The problem is thatstream.GetBuffer() is really just the internal buffer. The data represented may

  • start at a different index than 0 (if a non-zeroindex is passed in the constructor)
  • be less than the full buffer

The primitive solution is to useToArray, but that creates another copy. Since .NET 4.6, there is also aTryGetBuffer API that outputs anArraySegment containing all necessary information. I'll push a commit with the respective adjustment.

Copy link
Member

Choose a reason for hiding this comment

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

Fixed in6e37568

@lostmsulostmsu merged commit32051cb intopythonnet:masterMay 10, 2024
27 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@filmorfilmorfilmor left review comments

@psyfrettpsyfrettpsyfrett left review comments

@lostmsulostmsulostmsu requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@BadSingleton@filmor@lostmsu@psyfrett
[8]ページ先頭
©2009-2025 Movatter.jp