MerlinofMines / EasyCommands

Github Repository for Ingame Scripts built by MerlinofMines. Uses MDK to Deploy to SpaceEngineerse
GNU General Public License v3.0
8 stars 3 forks source link

Attempt at Consolidating Property Support #224

Closed MerlinofMines closed 2 years ago

MerlinofMines commented 2 years ago

This is my first attempt at consolidating property support, which removes VariableProperty and Direction.

Upon inspection, it's likely a bridge too far, or at least to start with. There are too many cases where "moving" a property requires special property handler support in the new system, that wasn't required in the old.

This will serve as a template though for a smaller change to add multi-property support, without removing Directional support.

Overview

This PR is attempting to remove the distinction between "Directions" (UP/DOWN/LEFT/RIGHT) and "PROPERTIES", by converting directions to properties, and allowing property resolution over a set of properties (vs just one).

The end state is that multiple propertyIds can be used to provide specificity on what you are trying to read/write from a property, where the structure sounds very natural. For example:

#2 Properties, "Deviation" and "Angle"
set the "Custom TurretController" deviation angle to 5

#2 Properties, "Steering" and "Inverted"
if the "Front Wheels" steering is inverted
  Print "My steering is inverted"

As part of this change I also

Specific Changes

Challenges

Property Resolution

Previously properties were looked up by the property id (as a string). With multi-property support lookup, this no longer works. Additionally, the intention is that a set of properties should resolve to the same "handler". To support this, some form of hash is needed which is consistently computed regardless of ordering (property1+property2 = property2 + property1). Example:

# These should use the same property handler
invert my "Test Wheel" steering
if my "Test Wheel" steering is inverted

I think the best way to do this is to sum up the hashcodes (unsigned and converted to long to avoid stack overflow) and use the sum as the hash. I started with XOR but realized that property1 + property2 + property1 would equal property2, which is a problem. Needs to be updated to use sums.

Dynamic Properties

Previously dynamic property support assumes only 1 input property. Updates were needed in Properties to support multiple dynamic properties being specified. In addition, the propertyId resolution for TerminalBlockHandler needed to change in case there happened to be multiple propertyIds passed. My simple solution was to just concat them together (almost guaranteed to fail), and update the output "does not have propertyId" to also do this. Note that I changed the output to always use the propertyWord instead of falling back to propertyType. The constructor for PropertyValue now defaults propertyWord to propertyType if not passed, so propertyWord is always present.

Directional Support

Previously you could increase or decrease a property by "moving" it in a direction. This was particularly useful for Raising and Lowering property values. Example:

raise the "Test Piston" height to 5

Getting rid of directions, this is becoming a bit more challenging. Previously properties that didn't have "directions" would simply ignore the fact that you passed a direction, and so had the effect of setting the property value. However, in the new world, "raise height" equates to "Property.UP + Property.LEVEL", which must be explicitly mapped if you want to support this.

In addition, previously if you did not specify a specific value, (like 5), the effect would "move" a property, which for most properties had the effect of incrementing/decrementing unless you overrode the behavior on the block handler.

#Overridden to call "extend"
raise the "Test Piston"

#Incremented by the default amount
raise the "Test Beacon" range

It's mostly just the "raise" and "lower" keywords but there are a few other challenges as well. Here's an example list:

raise the piston //Extend
raise the piston to 5 //Set piston height to 5
raise the piston height //Extend
raise the piston height to 5 //Set piston height to 5
raise the piston upper limit //Increment upper limit by default amount for limit property
raise the piston upper limit to 6 //set upper limit property to 6
raise the piston lower limit //increment lower limit by default amount for limit property
raise the piston lower limit to 4 //set lower limit property to 4

set the piston to 5 //set piston height to 5
set the piston height to 5 //set piston height to 5
set the upper piston limit to 6 //set upper limit to 6
set the piston lower limit to 4 //set lower limit to 4

lower the piston//Retract the piston
lower the piston height //Retract the piston
lower the piston limit to 6 //set the piston limit property to 6
lower the piston upper limit to 4 //set the piston upper limit property to 4
lower the piston upper limit//lower the piston upper limit property by the default amount for the limit property

reverse the piston //reverse the piston
reverse the piston height //reverse the piston
reverse the piston velocity //velocity *= -1

Attribute Property Lookup

Previously the attribute lookup for the Assembler & Cargo was easy, since there was only 1 attribute. Now you need to look through each supplied PropertyValue and find the first attributeValue. I took the opportunity to consolidate the "getRequestedAmount" and "GetItemFilteR" methods into a single one, but this is not strictly necessary.

Possible Solutions

One thought I had was to re-map "raise" to a specific new type of parameter, and look for this type of parameter in the BlockCommand processor. If "raise" is included along with a variable, ignore the "UP" property and call. If no value is set, add the "Up" property to the supplier.

Lower is a bit more tricky, since lower could be an action, or just a Property:

#Action
lower the "Test Piston"

#Just a property specifieer
set the "Test Piston" lower limit to 5

I think we might be able to add a rule that says "if Lower, and lower is the first parameter, then it is an action, otherwise it just a property specifier". This might also be usable for Raise, actually. If the first parameter is "Up" or "Down" property, convert it to the action increment form?

With these two changes, I think we will have backwards compatibility with the existing script?

jgersti commented 2 years ago

some comments:

> Property Resolution

Are all combination independent of order? If we use hashes we need to check that there are no possible hash collisions. Partial matches might also be of interest.

> Directional Support

I guess lower should then also take the meaning of retract (assignment + down)? Right now lower is only used as directional keyword.

> Possible Solutions

Splitting Actions and Properties on a more fundamental level might be the way to go but might be difficult to resolve.

Resolution by order might need some rule processing to reduce (transform/eliminate) the given properties to get the desired results/match.

edit: The way the IDataProcessors are processed means that eitherList and leftList processors are difficult to work with if its contents gets used / ordering matters. leftList is filled in reverse order. eitherList get filled first with the matches on the right side and then with the matches on the left side in reverse. Both behaviours did not matter previouosly since either the content did not matter or the processor was never used.

MerlinofMines commented 2 years ago

My initial take is that multi-property support is not dependent on ordering. If we identify a case where it is, then we could adjust and have ordering matter but have combinations added by default when you add multi-properties (not added if already present). This would imply that properties are sets are not dependent on orderunless you specifically override them.

I think we hold off on that for now though, as you are correct that the way properties are resolved by the processors has to be adjusted as well, and it's not clear if this use case actually exists yet.

lower is often only a property, such as set the "Test Piston" lower limit to 5. I like the rule of "if it's the first parameter, treat as an action, otherwise it's only a property.