Renamed main MicrobeSpecies organelles to ModifiableOrganelles#6623
Conversation
…replaced with ReadonlyOrganelles
|
edit: quickly fixed the ThriveTest.sln and jetbrains DevCentre requirements, sorry for not noticing sooner! |
|
Does this fully change all? I ask because you haven't marked this as "closing" the issue, you just linked the issue. If you did fully solve the issue you should write |
|
Also is this meant to be a draft because as far as I can tell the code cannot compile currently? |
Thrive.csproj
Outdated
| @@ -1,5 +1,5 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| <Project Sdk="Godot.NET.Sdk/4.5.0"> | |||
| <Project Sdk="Godot.NET.Sdk/4.5.1"> | |||
There was a problem hiding this comment.
We cannot update to Godot 4.5.1 as it has a critical bug that will only be fixed in Godot 4.6
|
Oh thats my mistake, but this does compile and works fine im not sure why its saying otherwise, and the godot versions my bad i didnt see it listed on the setup guide, ill go through and recheck everything just to make sure |
|
Oh, it does compile but with warnings: We don't allow any compiler warnings in Thrive so those will have to be fixed as well. (the CI system treats them as build errors) |
|
I corrected the "closes" syntax as it still was not correct (it needs to be exact for Github to automatically detect it). |
|
The null propagation stuff is likely not your fault. .NET 10 offers a new feature related to that so all of our old code is triggering the issue. For some reason when I run the check it doesn't detect all of the problems at once which is why I've been fixing those piecemeal. |
Oh i see, maybe its something to do with me compiling on linux? But im not too sure |
|
How could that be? It's some kind of caching issue where the warnings are not reported until someone tries to change a file that would have the error. I did already do a new commit to master fixing up the files the CI check failed on your branch so if you update the branch then it should stop failing for them. |
…rganelles-to-modifiableorganelles
Alright thats grand thank you!
Im not sure, im so used to weird platform specific bugs because of my previous work being with c and c++ that i assumdd it wpuld be the same with c#, my bad! Ive just synced so hopwfully everything compiles properly now |
Oh yeah, C++ does do that. Luckily in my experience C# does not have that at all. It probably helps that we only use the dotnet compiler on each platform and not platform specific compilers. And funnily enough the C++ native module for Thrive did have a mac specific system linker bug that hit us and caused quite many headaches. That's now worked around with some changed build systems, but yeah I do still recall all kinds of stuff that goes wrong in C++ between platforms. But luckily we've not had that at all in C#.
Yep. It looks like the CI checsk now fail purely due to your changes on this branch (too long lines, some not cleaned up whitespace, and an unnecessary using). You probably also want to clean up all of the unrelated changes like |
|
Oh really? Thats odd, ill have a look and fix it when im back at my pc |
|
might be a silly question, but is there a way to get rider to detect the formatting issues so i dont have to commit to check them? in other news this should be the final commit |
|
alright so i've run the checks, turns out the only issues now are null propagation related |
|
You mean locally or...? This latest commit here is failing due to a few missing blank lines: https://dev.revolutionarygamesstudio.com/ci/1/build/16369/jobs/2 |
|
yeah i meant locally haha, finally got there, even if it took a while and some defiantly amateur level mistakes on my end |
There was a problem hiding this comment.
thats really odd, ill run sync again, hopefully should clear that up
| <ProjectReference Include="RevolutionaryGamesCommon\SharedBase\SharedBase.csproj" /> | ||
| <ProjectReference Include="third_party\Arch.Extended\Arch.System.SourceGenerator\Arch.System.SourceGenerator.csproj" | ||
| OutputItemType="Analyzer" ReferenceOutputAssembly="false" /> | ||
| <ProjectReference Include="third_party\Arch.Extended\Arch.System.SourceGenerator\Arch.System.SourceGenerator.csproj" OutputItemType="Analyzer" ReferenceOutputAssembly="false" /> |
There was a problem hiding this comment.
Is there a reason for this change?
There was a problem hiding this comment.
no, i dont believe i touched i just compiled it
There was a problem hiding this comment.
Okay, then you should revert this file as well.
There was a problem hiding this comment.
Old project backup files shouldn't be committed.
| public OrganelleLayout<OrganelleTemplate> ModifiableOrganelles { get; private set; } | ||
|
|
||
| public OrganelleLayout<OrganelleTemplate> ModifiableOrganelles => Organelles; | ||
| public OrganelleLayout<OrganelleTemplate> ReadonlyOrganelles => ModifiableOrganelles; |
There was a problem hiding this comment.
This doesn't look right... because this returns instances that are modifiable. Thus this basically isn't a real readonly view of the data. See how I've done the MulticellularSpecies.GameplayCells which is a readonly adapter that only allows readonly instances to be read.
And I just realized an even bigger problem here: The return type here is OrganelleLayout meaning that there's nothing that prevents someone from calling ReadOnlyOrganelles.Add(...) and that's just wrong, because we wouldn't be getting any safety improvement which was my main point why opened #6483
So, sorry if things weren't clear upfront.
There was a problem hiding this comment.
of course thats no worries! ill go through and correct it :)

Preface
this is my first PR so apologies for any formatting errors!
ive gone through and double checked everywhere however so everything here should be fine.
Brief Description of What This PR Does
This PR renames the MicrobeSpecies Organelles to ModifiableOrganelles, MicrobeSpecies ModifiableOrganelles (the original) to ReadonlyOrganelles for clarity.
ive also gone through and changed some of the unnecessary usages of Organelles to the readonly varient as they dont modify any data, thus dont need to have setter rights.
Related Issues
Closes #6483
Progress Checklist
Note: before starting this checklist the PR should be marked as non-draft.
break existing features:
https://wiki.revolutionarygamesstudio.com/wiki/Testing_Checklist
(this is important as to not waste the time of Thrive team
members reviewing this PR)
styleguide.
Before merging all CI jobs should finish on this PR without errors, if
there are automatically detected style issues they should be fixed by
the PR author. Merging must follow our
styleguide.