fix: Rollback breaking changes made to the pixel format public API

[Eideren]

PR Details

Users shouldn't have to fix syntactic issues when upgrading versions, rolling back related changes.
Feel free to setup an upgrader if you want to keep your changes in @Ethereal77 and I'll close this PR.
Also, are there any other breaking changes in the public API @Ethereal77 ?

Related Issue

Types of changes

• Docs change / refactoring / dependency upgrade
• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• My change requires a change to the documentation.
• I have added tests to cover my changes.
• All new and existing tests passed.
• I have built and run the editor to try this change out.

[Ethereal77]

Yes, there are more breaking changes. Some time ago, we discussed if this PR (and others of the same kind) would more appropriately merge into a future or 5.0 branch, and that it was acceptable then to break things. That's the why 😢

I can create upgraders to ease the pain, but I'm not very knowledgeable in how the upgraders work. Any idea where to start looking?

[Eideren]

@Ethereal77 Makes sense, I would rather we leave it as a last resort for syntax though, the editor does not handle loading uncompilable projects well enough right now. And for non-programming people, this breaking change makes a project effectively unusable.

Syntax changes are unfortunately not really handled well afair, have a look at how XenkoToStrideRenameHelper is used in the engine. Not sure how feasible making an upgrader for this case would be though, given how fragile those regex would have to be.

[Ethereal77]

I would rather we leave it as a last resort for syntax though, the editor does not handle loading uncompilable projects well enough right now.

If this is a breaking change so big to create that kind of friction, I propose what I already proposed (and some people agreed on), to create a separate branch for "future Stride", where all these kind of changes can go (like, for example, the cross-platform editor, the OpenGL deprecation, the new D3D12-only editor, the new shader compilation pipeline, etc.)
Then, merge those changes back to master only at specific tagged versions that are deemed to have "acceptable" breaking changes.

Even though I understand what difficulties can imply these syntax changes, I think it would be a pitty to leave out some of these. C# 14's "Extension everything" in particular can be very valuable to modernize the codebase and allow some very interesting things.

Edit: On a second thought, maybe instead of an upgrader, this could be an analyzer? You've seen that the change basically is removing a () from the extension invocation. There are some renames (like IsVideo to IsVideoFormat), but those we can duplicate and annotate with [Obsolete].

And for non-programming people, this breaking change makes a project effectively unusable.

Get what I'm about to say not as if they are less worthy than "programming people", but just as my opinion as someone that has been following gamedev topics since before editor-engines (like Unity), and based on the context that currently Stride requires far more coding understanding than other engines:

I can't talk for non-programming people, but I think Stride may not be the best engine for people that do not want to touch and understand the underlying code. Ideally, everything should just work forever without problems, but retrocompatibility usually imposes a very hard limit to evolution in complex projects, and in a game engine, which is a very complex project and one that needs to be always at the very edge of efficiency, it does more so.

Having said that, the best advice I can give to non-programming people that want to create a complex project touching little code with Stride is to stick to a version that works for them and not moving versions (or at least, only minor revisions, bug fixes, etc.)

...have a look at how XenkoToStrideRenameHelper is used in the engine.

Correct me if I'm wrong, but upgraders and renamers are not just for assets? (I mean, asset names/types, serialized properties, type changes and conversions, etc.)
If that's the case, an upgrader cannot upgrade code. Can it?

---

Anyway, sorry for the wall of text. 😁
Please understand that it’s a bit frustrating for me to see some of my changes undone, as with clearer planning and more alignment among us this could have been a smoother process.

[Eideren]

I also would rather we break users project if we have some critical issue that needs to be addressed, method calls being swapped into properties with the same functionality is not that.
I would advocate against these kinds of changes even if we had a future branch. If we had other syntax breaking changes that would actually provide some functionality or fix some critical issues, why not, but none of the changes we introduce in the future will prevent a project from being built, the opengl one for example will fallback to the appropriate API.

I'm also not saying that we shouldn't use extension everything, just that we can only use it in case where we wouldn't introduce a breaking change.

I can't talk for non-programming people, but I think Stride may not be the best engine for people that do not want to touch and understand the underlying code.

Almost no games are made solo, the editor is supposed to be for non-programmers to work with programmers, and we've had terrible adoption specifically because we have thousands of things breaking all the time, let's not add one more thing.

XenkoToStrideRenameHelper goes through c# file content to swap namespaces usages

Please understand that it’s a bit frustrating for me to see some of my changes undone, as with clearer planning and more alignment among us this could have been a smoother process.

I totally get that, I wouldn't want to have my work undone either, you've done some incredible work for the engine, I very much respect and am glad of the many weeks of work you put into these changes.

I think that's just the limitation of how we're organized, we barely have the time to review, actually getting into those kinds of conversations seems very unlikely.

[Ethereal77]

I'm also not saying that we shouldn't use extension everything, just that we can only use it in case where we wouldn't introduce a breaking change.

I agree with this half-way. In my opinion, having new features implemented in the future with "extension everything" while we still have things using the old syntax is kind of inelegant, and I foresee questions by users regarding this.

Almost no games are made solo, the editor is supposed to be for non-programmers to work with programmers, and we've had terrible adoption specifically because we have thousands of things breaking all the time, let's not add one more thing.

Well, of course not. But in this projects not made solo, the responsibility to know the syntax changes (in scripts) fall precisely in the programmers, no? Anyway, I understand that there will be cases of artists or designers that would want to edit scripts themselves and feel confused about syntax errors where previously there were none.

XenkoToStrideRenameHelper goes through c# file content to swap namespaces usages

If that's the case, I'll see if it is possible to craft something that removes the () from those extension calls. Maybe that is enough to circumvent the problem.

I totally get that, I wouldn't want to have my work undone either, you've done some incredible work for the engine, I very much respect and am glad of the many weeks of work you put into these changes.

I think that's just the limitation of how we're organized, we barely have the time to review, actually getting into those kinds of conversations seems very unlikely.

I appreciate that. Thank you.
I think it would be wise if we at least stablish some kind of protocol when integrating changes into the engine, to avoid potential conflicts and regressions, to avoid having to do rollbacks, etc.
I insisted in my PR that it needed to be tested and used in real use cases at least some time before considering it safe to merge. I have been trying to get the tests green most of the time, but those are synthetic tests.
Maybe something to discuss in the next meeting?

Anyway, you decide if you merge the rollback now (if urgent), or wait until a (possible?) upgrader can be done.

[Eideren]

Let's leave this as is for now until after you've looked into whether an upgrader would be feasible. An alternative option would be to change the nomenclature of those properties and mark the extension methods as obsolete.

[Ethereal77]

I have been trying different things. What is your opinion on a "two-stage obsoletion" strategy? Let me explain:

• Any extension method that now conflicts with the new syntax is marked with, for example:

  [Obsolete("BlockSize() is obsolete and will be deprecated in a future version. It is to be replaced by the extension property BlockSizeInBytes. You can try the new syntax by importing the Experimental namespace.")]
  

  Note it has a different name for now. More on that later.

• At the same time, we have the conflicting extensions in another class in an Stride.Graphics.Experimental namespace.
  To avoid ambiguity and allow to use the new syntax immediately, we use a different (albeit readable) name temporarily, i.e., BlockSize -> BlockSizeInBytes, or BlockWidth -> BlockPixelWidth, etc.

• Then, in the next version of Stride, we effectively remove the old extension method syntax, move the new syntax out of Stride.Graphics.Experimental, and do the following:

   [Obsolete("BlockSizeInBytes will be renamed to its previous name (BlockSize) in the next version. This is a temporary workaround to allow both syntaxes to coexist during the transition period.")]
   public int BlockSizeInBytes => /* Whatever */;
   public int BlockSize => /* Whatever */;
  

What do you think?

[Eideren]

Yep, sounds good to me

[Eideren]

Leaving this as a draft while Ethereal takes care of the above, will close it as soon as his opens.