GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Expose serialization api by BadSingleton · Pull Request #2336 · pythonnet/pythonnet

Conversation

@BadSingleton

@BadSingleton BadSingleton commented Mar 7, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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 to AUTHORS
  • Updated the CHANGELOG

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
BadSingleton added 2 commits February 6, 2024 15:47
@BadSingleton
Expose an API for users to specify their own formatter
d7f31bf 
Adds post-serialization and pre-deserialization hooks for additional
customization.
@BadSingleton
Add API for capsuling data when serializing
aa37f89 
* And revert the breaking change of the FormatterType removal
* Add CHANGELOG entry
@BadSingleton BadSingleton mentioned this pull request Mar 7, 2024
Best effort serialization #1892
Closed
3 of 5 tasks
filmor
filmor reviewed Mar 7, 2024
View reviewed changes
Comment thread src/runtime/StateSerialization/RuntimeData.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
@filmor

filmor commented Mar 12, 2024

Copy link
Copy Markdown
Member

@lostmsu any notes?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lostmsu
lostmsu reviewed Mar 12, 2024
View reviewed changes
Comment thread src/runtime/StateSerialization/RuntimeData.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/runtime/StateSerialization/RuntimeData.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/runtime/StateSerialization/RuntimeData.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/runtime/StateSerialization/RuntimeData.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/runtime/StateSerialization/RuntimeData.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/runtime/StateSerialization/RuntimeData.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/runtime/StateSerialization/RuntimeData.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/runtime/StateSerialization/RuntimeData.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
@lostmsu

lostmsu commented Mar 12, 2024

Copy link
Copy Markdown
Member

Add some tests.
BadSingleton reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
BadSingleton added 2 commits March 17, 2024 11:05
@BadSingleton
Part one of review fixes
647c75c
@BadSingleton
Adds tests for custom serialization
02158f3
BadSingleton
BadSingleton commented Mar 29, 2024
View reviewed changes
Comment thread tests/domain_tests/TestRunner.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
@lostmsu

lostmsu commented Mar 29, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BadSingleton

BadSingleton commented Mar 29, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@filmor
Add NoopFormatter and fall back to it if BinaryFormatter is not avail…
a058900 
…able
@filmor

filmor commented Apr 26, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BadSingleton

BadSingleton commented Apr 26, 2024

Copy link
Copy Markdown
Contributor Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lostmsu
lostmsu requested changes Apr 28, 2024
View reviewed changes
Comment thread src/runtime/StateSerialization/RuntimeData.cs Outdated
/// <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();

@lostmsu lostmsu Apr 28, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@BadSingleton BadSingleton Apr 29, 2024

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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 using Length not Capacity, so it should not be an issue, no?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@filmor filmor May 6, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@filmor filmor May 6, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Fixed in 6e37568

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@filmor
Use TryGetBuffer instead of GetBuffer
6e37568
@lostmsu
lostmsu merged commit 32051cb into pythonnet:master May 10, 2024
@dependabot dependabot Bot mentioned this pull request Jan 16, 2026
Bump pythonnet from 3.0.3 to 3.0.5 mattemangia/Deep3DStudio#99
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@filmor filmor filmor left review comments
@lostmsu lostmsu lostmsu requested changes
+1 more reviewer
@psyfrett psyfrett psyfrett left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@BadSingleton @filmor @lostmsu @psyfrett
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.