Change the movable contents to special list, and make view include Contents #2050
Conversation
SaphireLattice
force-pushed
the
fix/movable-contents
branch
from
d6ee43d to
7d320d5
Compare
| foreach (var content in centerContentsList.GetValues()) { | ||
| view.AddValue(content); | ||
| } |
There was a problem hiding this comment.
| foreach (var content in centerContentsList.GetValues()) { | |
| view.AddValue(content); | |
| } | |
| view.AddRange(centerContentsList.GetValues()); |
pretty sure this can just be an AddRange()?
There was a problem hiding this comment.
view is a DreamList which does not have any range-adding methods. Closest would be OperatorAppend() but that should probably be only be used by DM.
|
Oh yeah, remembered another thing I have no damn clue as to the stability of the iteration keys and trying to get items "by index" where it's just another iterator again and uhhh... yeah. It didn't seem to explode at least? Like, taking off stuff and moving things around in hands worked fine. Didn't check it with backpacks too much though. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -1263,6 +1271,91 @@ | |||
| } | |||
| } | |||
|
|
|||
| // mob.contents, obj.contents list | |||
| public sealed class MovableContentsList(DreamObjectDefinition listDef, DreamObjectMovable movable) : DreamList(listDef, 0) { | |||
| private readonly DreamObjectMovable _movable = movable; | |||
Check notice
Code scanning / InspectCode
Replace with primary constructor parameter Note
| // mob.contents, obj.contents list | ||
| public sealed class MovableContentsList(DreamObjectDefinition listDef, DreamObjectMovable movable) : DreamList(listDef, 0) { | ||
| private readonly DreamObjectMovable _movable = movable; | ||
| public override DreamValue GetValue(DreamValue key) { |
Check warning
Code scanning / InspectCode
Incorrect blank lines: Blank lines are missing elsewhere Warning
| throw new Exception($"Invalid index into movable contents list: {key}"); | ||
|
|
||
|
|
||
| if (index < 1 || index > _movable.ChildCount) |
Check warning
Code scanning / InspectCode
Incorrect blank lines: Blank lines are redundant elsewhere Warning
| throw new Exception($"Out of bounds index on movable contents list: {index}"); | ||
|
|
||
|
|
||
| using (var childEnumerator = _movable.ChildEnumerator) { |
Check warning
Code scanning / InspectCode
Incorrect blank lines: Blank lines are redundant elsewhere Warning
| throw new Exception($"Out of bounds index on movable contents list: {index}"); | ||
|
|
||
|
|
||
| using (var childEnumerator = _movable.ChildEnumerator) { |
Check notice
Code scanning / InspectCode
Convert into 'using' declaration Note
|
|
||
| using (var childEnumerator = _movable.ChildEnumerator) { | ||
| while (index >= 1) { | ||
| var current = childEnumerator.MoveNext(out EntityUid child); |
Check warning
Code scanning / InspectCode
Unused local variable Warning
| public override List<DreamValue> GetValues() { | ||
| List<DreamValue> values = new List<DreamValue>(_movable.ChildCount); | ||
|
|
||
| using (var childEnumerator = _movable.ChildEnumerator) { |
Check notice
Code scanning / InspectCode
Convert into 'using' declaration Note
| if (!value.TryGetValueAsDreamObject<DreamObjectMovable>(out var movable)) | ||
| return false; | ||
|
|
||
| if (!movable.TryGetVariable("loc", out var _locVariable)) |
Check warning
Code scanning / InspectCode
Inconsistent Naming Warning
| if (!movable.TryGetVariable("loc", out var _locVariable)) | ||
| return false; | ||
|
|
||
| if (!_locVariable.TryGetValueAsDreamObject<DreamObjectAtom>(out var _loc)) |
Check warning
Code scanning / InspectCode
Inconsistent Naming Warning
| if (!value.TryGetValueAsDreamObject<DreamObjectMovable>(out var movable)) | ||
| throw new Exception($"Cannot add {value} to movable contents"); | ||
|
|
||
| movable.SetVariable("loc", new (_movable)); |
There was a problem hiding this comment.
This should use the SetLoc() method on movable instead. Feel free to make it public.
| public virtual void AddValueRange(DreamList valueRange) { | ||
| _values.AddRange(valueRange.GetValues()); | ||
| } | ||
|
|
||
| public virtual void AddValueRange(IEnumerable<DreamValue> valueRange) { | ||
| _values.AddRange(valueRange); | ||
| } |
There was a problem hiding this comment.
Adding helper functions like this that perform directly on _values should be avoided. These would have to be overridden on every DreamList type to handle their special behaviors.
| throw new Exception($"Out of bounds index on movable contents list: {index}"); | ||
|
|
||
|
|
||
| using (var childEnumerator = _movable.ChildEnumerator) { |
There was a problem hiding this comment.
It should be noted that ChildEnumerator is an enumerator of a HashSet which has no defined order. This means the order could theoretically change at any time, breaking this.
I don't think this is a new bug so it doesn't really need to be worried about in this PR.
| if (!value.TryGetValueAsDreamObject<DreamObjectMovable>(out var movable)) | ||
| throw new Exception($"Cannot remove {value} from movable contents"); | ||
|
|
||
| movable.SetVariable("loc", DreamValue.Null); |
There was a problem hiding this comment.
This doesn't verify that this movable was even in our contents to begin with.
Also, use SetLoc() here as well.
There was a problem hiding this comment.
Oh forgot about that. Guess it would be a no-op if it isn't?
And thanks for telling about SetLoc, was not aware! Swear I've seen this pattern elsewhere.
| if (!movable.TryGetVariable("loc", out var _locVariable)) | ||
| return false; | ||
|
|
||
| if (!_locVariable.TryGetValueAsDreamObject<DreamObjectAtom>(out var _loc)) | ||
| return false; | ||
|
|
||
| return _loc == _movable; |
There was a problem hiding this comment.
DreamObjectMovable.Loc can be used instead of indirectly going through variables.
| if (!movable.TryGetVariable("loc", out var _locVariable)) | |
| return false; | |
| if (!_locVariable.TryGetValueAsDreamObject<DreamObjectAtom>(out var _loc)) | |
| return false; | |
| return _loc == _movable; | |
| return dreamObject.Loc == movable; |
| @@ -19,6 +19,10 @@ public class DreamObjectMovable : DreamObjectAtom { | |||
|
|
|||
| private readonly TransformComponent _transformComponent; | |||
|
|
|||
| public readonly MovableContentsList Contents; | |||
There was a problem hiding this comment.
Move this up with the other public properties
| public TransformChildrenEnumerator ChildEnumerator => _transformComponent.ChildEnumerator; | ||
| public int ChildCount => _transformComponent.ChildCount; |
There was a problem hiding this comment.
Though, worrding that it's shitcode to have public ChildEnumerator and public ChildCount exposed by the DreamObjectMovable. Maybe I could just give the MovableContentsList its Transform component?
I would prefer for you to do this.
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
New list implementation was required to make
+=and such work. Not sure ifSetVarwould have helped? Actually didn't test if a reference to the list still updates the object on reference implementation, but it feels like it should. Then again, never know what weird shit might happen.Also made
view(...)includecontentsjust likerange(...)- these probably should share the implementation code, other than a boolean. They are pretty much the same, just with view culling.Oh, this fixes:
Though, worrding that it's shitcode to have
public ChildEnumeratorandpublic ChildCountexposed by theDreamObjectMovable. Maybe I could just give theMovableContentsListits Transform component?