Add cell position adjacency bonus calculations EXCEPT for GetPlayerDataSource()#6855
Add cell position adjacency bonus calculations EXCEPT for GetPlayerDataSource()#6855
Conversation
…DataSource() Still don't really know how to do it there, honestly.
…ataSource()" This reverts commit 2b2d049.
There was a problem hiding this comment.
Overall I think this is pretty good for the backend part of this feature, though the changes in this file are kind of looking very similar, so I think some of the methods here could be simplified so that they just look up some data and then call an overload. Would that be possible?
Also one slight naming thing is that you should clarify if the body plan is the gameplay or editor body plan (as your code only works for the editor body plan variant and will fail for the gameplay body plan).
There was a problem hiding this comment.
so I think some of the methods here could be simplified so that they just look up some data and then call an overload. Would that be possible?
I'm not exactly sure what you mean by this, and I don't really see any way to simply these methods. CMIIW though, of course.
(as your code only works for the editor body plan variant and will fail for the gameplay body plan)
This seems a bit too important to just hide in parentheses.
There was a problem hiding this comment.
I mean it looks like the index method could just get the cell type from the index and then call the variant that takes in a cell template directly? I did not try to do that rewrite so I might be missing some small detail but to me that seems like a plausible code refactor.
This seems a bit too important to just hide in parentheses.
I assumed you were aware of that... You'll want to check the differences between ModifiableGameplayCells and ModifiableEditorCells as I think that yeah your algorithm works for one but does not work for the other, so it should be clarified with variable naming and maybe even putting an XML comment with a warning about that that the algorithm only works for one cell layout variant.
Huge thanks to Rathalos (Accidental-Explorer) here!
|
|
||
| foreach (var cell in bodyPlan) | ||
| { | ||
| if (cellInBodyPlan!.CellType == cell.Data!.CellType |
There was a problem hiding this comment.
Why did you make the parameter nullable if you just use null suppression on it? This is the incorrect way to use null suppression; it should only be overridden when you are sure it is safe to do. We have null suppression enabled so that the compiler can help us notice potential bugs in code and need to write the proper handling code (or modify the parameter definitions so that our caller has to get valid data for us).
| IReadOnlyIndividualLayout<IReadOnlyCellTemplate> bodyPlan) | ||
| { | ||
| var bonus = 0.0f; | ||
| var plan = bodyPlan.ToList(); |
There was a problem hiding this comment.
Temporary memory should be avoided. So the looping needs to be done in a different way here.
| var cellInBodyPlan = bodyPlan[cellIndexInBodyPlan]; | ||
|
|
||
| foreach (var cell in bodyPlan) | ||
| { | ||
| if (cellInBodyPlan.Data!.CellType == cell.Data!.CellType | ||
| && cell.Position.DistanceTo(cellInBodyPlan.Position) == 1) | ||
| { | ||
| bonus += Constants.CELL_ADJACENCY_SPECIALIZATION_BONUS; | ||
| } | ||
| } |
There was a problem hiding this comment.
This still looks very similar to me...
Like I think the code could be like:
float GetAdjacencyWithAFancyName(CellTemplate thing, others...){
loop logic here
}
float GetAdjacencyWithAFancyName(int index, others...){
var resolved = others[index];
return GetAdjacencyWithAFancyName(resolved, others...);
}
So that way there's less code repetition.
as that will no longer be possible with the position-based specialization applying
2 participants
Brief Description of What This PR Does
This PR adds the calculations for cell position adjacency bonuses EXCEPT for in
GetPlayerDataSource(). The bonus is currently 4%.This is but a subset, as the mentioned exception has to be added, as well as the GUI display.
Related Issues
#6764 (shouldn't close it yet)
Extra Context
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.