Refactoring changes in Stride.GameStudio 2

[Jklawreszuk]

PR Details

This another pack of changes around GameStudio
List of changes

• use field keyword to shortify code
• update crash report page, so that refers to GH Issues (took from fix : Metrics - Remove all data collection from Stride.Engine #2261)
• remove useless isCoreCLR variable refering to legacy mono runtime

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.

[Jklawreszuk]

Right forgot to test Stride.Launcher let me fix this :D

[Jklawreszuk]

The tests should now pass 😄

[Kryptos-FR]

Ok, I'll try to do a review very soon.

Edit: likely this weekend. I couldn't find the time during the week.

[Kryptos-FR]

Overall LGTM, but I'd like to point out potential conflicts with the xplat-editor branch that I would prefer to be minimized. That said, it might not be easy to adapt in this branch, so let me know. Worst case, I will manage those conflicts later when I merge master into xplat-editor.

There a few other nit picks here and there.

[Kryptos-FR]

I made similar changes for the crash report in the xplat-editor branch. To limit future conflicts, could you reuse the same changes as much as possible?

Note in the xplat-editor branch I use Avalonia with MVVM so obviously not everything can be kept the same.

[Jklawreszuk]

I converted the project from WinForms to WPF in a similar manner to your branch ,though is not identical. Perhabs I could make a PR later to your branch once my changes will merge ?

[Kryptos-FR]

I will have a look. Thanks, it's already of help what you did :)

[Kryptos-FR]

I asked Claude AI to cross-check the .pot and .po files against the code. The results are good, but we do have translations that are out of sync. It's not an issue with this PR, but we should consider taking care of it at some point (maybe for Stride 5.0 when we will have the new xplat editor).

For reference, Claude AI generated two python scripts that can be very useful to detect such issues:

• validate_translations.py
• check_po_consistency.py

Claude AI analysis report (summarized)

The PR's translation changes are technically correct:

• ✅ All line references are accurate
• ✅ The .pot files correctly reflect the current source code
• ⚠️ Some .po files need updating by translators (standard maintenance)

The 3,392 inconsistencies are not errors in this PR, but rather indicate that some translation files haven't been synchronized yet with the latest source code changes.

[Kryptos-FR]

LGTM thanks.

[Kryptos-FR]

@Jklawreszuk can we merge this or did you want to make more changes?

[Jklawreszuk]

@Kryptos-FR I dont think so, feel free to merge them 👌

[xen2]

@Kryptos-FR Wouldn't it be better to merge as a branch?

First, it gives more history detail (see what line belong to what changes)
(I prefer that much more)

But more importantly for editor commits, I think that would also make your life easier for merging with your xplat branch:

• if merging tip of master is hard due to a specific commit, you can merge more incrementally with several steps before/right at the conflicting commits.
• you can even cherry-pick part after conflicting commit to deal with conflicting commit later if needed.
• Larger commits are annoying to merge if there are some conflicting parts (have to deal with lot of unrelated conflicts, can't test anything until it compiles again). Also you can't go step by step and difficult to trace back why anything broke in case of a merge mistake.

In a nutshell, it gives you much finer granularity for merging.

Of course, doesn't apply if it's a single small feature with many trivial commits like "fix typo" "fix again previous error" and such, those should be squashed -- use best judgement!
But in this case, with 5/6 separate small refactors, I think it makes much more sense.

[Kryptos-FR]

@xen2 I thought the same for a long time. However, it's not necessary.

That's because GitHub keeps a hidden ref for all PRs even after they are merged and the original source branch is deleted. On top of that, using squash merges makes the history cleaner. And usually in a branch used for a PR, not all commits are clean (some don't even compile).

So in the end:

• I always squash merge.
• When merging back into a branch (like the xplat editor), I still have the option to either
  • merge at once if the conflicts are easy to resolve (or no conflicts)
  • merge incrementally from the commits from the corresponding ref of the PR, and then either rebase, or reset "mixed"

We can have the best of both worlds:

• a clean history
• a fallback way to merge incrementally or even revert partially a previous PR from a specific commit from the PR using the ref

[xen2]

Agree on some points:

• squash when useless commits or small PR -- it makes sense to have everything as a single commit
• it's nice to have clean history whenever possible

But disagree on some others:

• History details is not just for merge. One of the most important use is for git blame, and after a squash it's all gone.
  (which is fine if we don't need it, but for the xplat branch, or the very large feat: Convert from SharpDX to Silk.NET #1715 branch, I would say it's vital to keep proper history in the repo)

• I don't expect people will go check on github website for old PR; they'll just not do it and proceed with less information (vs a simple git blame, available within your IDE)
  I know I won't want to open github website all the time for that, the workflow would suck.
  Comments won't be enough if you can't see the full changeset of a specific feature with some other associated files (if your change is mixed with 100s of other commits due to a squash, that's lot of lost information that we could have kept for almost free).

• We would rely on github, a specific provider, for the real history. If you just checkout the git repo, we won't have proper history. I am very happy with github but I wouldn't tie our history to a provider, that's what git is for.

• If some commits are not working, it's not a problem as long as they are in a branch and merged back in the target branch (using right side, not like I did by mistake on feat: Convert from SharpDX to Silk.NET #1715 ^^). What we want to avoid is a rebase without squash if some commits are not working. A merge is enough to ensure tools like regression checking (i.e. git bisect) will work properly on the branch you merge as each commit should always work.

• IMHO, proper history/details (when it makes sense) is much higher priority than having "perfect timeline" (without merge) just for the sake of it in git. We can try to achieve it whenever it makes sense, but it's still secondary (but I understand the appeal, I personally do spend sometime way too much time doing lot of interactive rebase with fixup in my branches, and ideally try to keep it in good enough shape to rebase instead of merge, but it's not always feasible and is sometime making me waste hours).

Also, in general, I don't believe in a single approach fits every use case (i.e. always merge or always rebase or always squash).
We can use some common sense and figure out what's best in each case:

• Small/mini PR or PR with lot of disorganized commits => squash rebase
• Larger PR with well separated commits => rebase without squash or merge (to keep useful history info/git blame)
  It often happen as you do a PR that you have to fix something else; ideally it should be another PR but sometimes it's really annoying to work with separating them cleanly and add lot of overhead on dev vs just having 2/3 separate commits (that can be reordered and squashed as you work on each). Of course, it should be done only if the PR was cleanly separated. Or for very large PR where a more detailed history is important (cf feat: Convert from SharpDX to Silk.NET #1715 or xplat editor branch).
  • Rebase should be used only if every step is working (for nicer history look) otherwise regression checking (i.e. git bisect won't work). Another case against is when rebasing is too tedious due to conflicts (need to solve them for every commit, and check everything is working at every step as well, rather than once for the merge commit). I wouldn't put too much burden on developer and say a merge is better if he doesn't want to waste time on rebasing (or doesn't know how).

Since this choice (squash/rebase/merge) is left to one of Stride maintainer (and not to the PR writer who might not know about those details), we can make sure to be somewhat consistent and adjust to each PR depending on size and actual commit list (i.e. if lot of trash, simply squash+rebase)

If we follow your reasoning, do you intend to merge the whole xplat branch as a single commit? I rather hope not :)
Personally I would never have squashed the #1715 branch, there is way too much detail that I absolutely need to keep. It'll be very useful every time I or somebody else have to work again in the future on the graphics API (esp. all the details on Vulkan/D3D12, sometimes it's quite tricky).

[xen2]

PS: I've just discovered the "Show first parents" option on git extensions (Ctrl+Shift+S or the last item on the toolbar next to filter), it's awesome :)
I suspect other tools might have a similar option.
It makes a merge look like a squash+rebase.

[Kryptos-FR]

The PRs and their branches can be seen locally, not needed to go on GitHub or to be even connected (once fetched). That what I meant with "GitHub creates a special ref".

Add this to your local .git/config (second fetch line):

[remote origin]
    url = https://github.com/stride3d/stride.git
    fetch = +refs/heads/*:refs/remotes/origin/*
    fetch = +refs/pull/*/head:refs/remotes/origin/pr/*

That way all PR (even those merged) are fetched locally with their whole history (even if it was squash merges).