Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Support for (most) 1.8 maps #7631

Merged
merged 2 commits into from
Sep 13, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package games.strategy.engine.data;

import static com.google.common.base.Preconditions.checkNotNull;
import java.util.Optional;

/** A game data change that captures a change to an attachment property value. */
public class ChangeAttachmentChange extends Change {
Expand All @@ -10,7 +10,7 @@ public class ChangeAttachmentChange extends Change {
private final Object newValue;
private final Object oldValue;
private final String property;
private boolean clearFirst = false;
private final boolean clearFirst;

/**
* Initializes a new instance of the ChangeAttachmentChange class.
Expand All @@ -21,13 +21,13 @@ public class ChangeAttachmentChange extends Change {
*/
public ChangeAttachmentChange(
final IAttachment attachment, final Object newValue, final String property) {
checkNotNull(attachment, "null attachment; newValue: " + newValue + ", property: " + property);

attachedTo = attachment.getAttachedTo();
attachmentName = attachment.getName();
oldValue = attachment.getPropertyOrThrow(property).getValue();
this.newValue = newValue;
this.property = property;
this(
attachment.getAttachedTo(),
attachment.getName(),
newValue,
attachment.getPropertyOrThrow(property).getValue(),
property,
false);
}

/**
Expand All @@ -38,15 +38,14 @@ public ChangeAttachmentChange(
final IAttachment attachment,
final Object newValue,
final String property,
final boolean resetFirst) {
checkNotNull(attachment, "null attachment; newValue: " + newValue + ", property: " + property);

attachedTo = attachment.getAttachedTo();
clearFirst = resetFirst;
attachmentName = attachment.getName();
oldValue = attachment.getPropertyOrThrow(property).getValue();
this.newValue = newValue;
this.property = property;
final boolean clearFirst) {
this(
attachment.getAttachedTo(),
attachment.getName(),
newValue,
attachment.getPropertyOrThrow(property).getValue(),
property,
clearFirst);
}

/**
Expand All @@ -59,13 +58,16 @@ public ChangeAttachmentChange(
final Object newValue,
final Object oldValue,
final String property,
final boolean resetFirst) {
this.attachmentName = attachmentName;
final boolean clearFirst) {
this.attachmentName =
Optional.ofNullable(attachmentName)
.map(name -> name.replaceAll("ttatch", "ttach"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean we all know why this exists, but it would be nice documenting it here in the code nevertheless

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same on other places

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in: #7661

.orElse(null);
attachedTo = attachTo;
this.newValue = newValue;
this.oldValue = oldValue;
this.property = property;
clearFirst = resetFirst;
this.clearFirst = clearFirst;
}

public Attachable getAttachedTo() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,9 @@ public void setAttachedTo(final Attachable attachable) {

@Override
public String getName() {
return name;
return Optional.ofNullable(name)
.map(attachmentName -> attachmentName.replaceAll("ttatch", "ttach"))
.orElse(null);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public Map<String, IAttachment> getAttachments() {

@Override
public void addAttachment(final String key, final IAttachment value) {
attachments.put(key, value);
attachments.put(key.replaceAll("ttatchment", "ttachment"), value);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -467,21 +467,21 @@ private void parseProperties(final PropertyList propertyList) {
.orElseGet(() -> Strings.emptyToNull(current.getValue()));

final boolean editable = current.isEditable();
final String property = current.getName();
final String propertyName = LegacyPropertyMapper.mapPropertyName(current.getName());
if (editable) {
parseEditableProperty(current, property, value);
parseEditableProperty(current, propertyName, value);
} else {
if (current.getBooleanProperty() != null) {
properties.set(property, Boolean.parseBoolean(value));
properties.set(propertyName, Boolean.parseBoolean(value));
} else if (current.getStringProperty() != null) {
properties.set(property, value);
properties.set(propertyName, value);
} else {
try {
final int integer = Integer.parseInt(value);
properties.set(property, integer);
properties.set(propertyName, integer);
} catch (final NumberFormatException e) {
// then it must be a string
properties.set(property, value);
properties.set(propertyName, value);
}
}
}
Expand Down Expand Up @@ -900,7 +900,11 @@ private List<Tuple<String, String>> setOptions(
final List<Tuple<String, String>> results = new ArrayList<>();
for (final AttachmentList.Attachment.Option option : options) {
// decapitalize the property name for backwards compatibility
final String name = decapitalize(option.getName());
final String name = LegacyPropertyMapper.mapLegacyOptionName(decapitalize(option.getName()));
if (LegacyPropertyMapper.ignoreOptionName(name)) {
continue;
}

if (name.isEmpty()) {
throw new GameParseException(
"Option name with zero length for attachment: " + attachment.getName());
Expand All @@ -912,7 +916,8 @@ private List<Tuple<String, String>> setOptions(
continue; // Skip adding option if contains empty foreach variable
}
final String valueWithForeach = replaceForeachVariables(countAndValue, foreach);
final String finalValue = replaceVariables(valueWithForeach, variables);
final String interpolatedValue = replaceVariables(valueWithForeach, variables);
final String finalValue = LegacyPropertyMapper.mapLegacyOptionValue(name, interpolatedValue);
try {
attachment
.getProperty(name)
Expand All @@ -928,8 +933,8 @@ private List<Tuple<String, String>> setOptions(
} catch (final Exception e) {
throw new GameParseException(
String.format(
"map name: '%s', Unexpected Exception while setting values for attachment: %s",
mapName, attachment),
"map name: '%s', Unexpected Exception while setting values for attachment: %s, %s",
mapName, attachment, e.getMessage()),
e);
}
results.add(Tuple.of(name, finalValue));
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
package games.strategy.engine.data.gameparser;

import games.strategy.triplea.Constants;
import lombok.experimental.UtilityClass;

/**
* This converts property names and values that were used in older map XMLs and updates those values
* to their newer versions. This is useful to allow the game code to only use the newer names and
* values while still being able to load older maps.
*/
@UtilityClass
class LegacyPropertyMapper {

static String mapLegacyOptionName(final String optionName) {
if (optionName.equalsIgnoreCase("isParatroop")) {
return "isAirTransportable";
DanVanAtta marked this conversation as resolved.
Show resolved Hide resolved
} else if (optionName.equalsIgnoreCase("isInfantry")
|| optionName.equalsIgnoreCase("isMechanized")) {
return "isLandTransportable";
} else if (optionName.equalsIgnoreCase("occupiedTerrOf")) {
return Constants.ORIGINAL_OWNER;
} else if (optionName.equalsIgnoreCase("isImpassible")) {
return "isImpassable";
} else if (optionName.equalsIgnoreCase("turns")) {
return "rounds";
}
return optionName;
DanVanAtta marked this conversation as resolved.
Show resolved Hide resolved
}

public String mapLegacyOptionValue(final String optionName, final String optionValue) {
if (optionName.equalsIgnoreCase("victoryCity")) {
if (optionValue.equalsIgnoreCase("true")) {
return "1";
} else if (optionValue.equalsIgnoreCase("false")) {
return "0";
} else {
return optionValue;
}
}
return optionValue;
}

public String mapPropertyName(final String propertyName) {
if (propertyName.equalsIgnoreCase("Battleships repair at end of round")
|| propertyName.equalsIgnoreCase("Units repair at end of round")) {
return Constants.TWO_HIT_BATTLESHIPS_REPAIR_END_OF_TURN;
} else if (propertyName.equalsIgnoreCase("Battleships repair at beginning of round")
|| propertyName.equalsIgnoreCase("Units repair at beginning of round")) {
return Constants.TWO_HIT_BATTLESHIPS_REPAIR_BEGINNING_OF_TURN;
}

return propertyName;
}

static boolean ignoreOptionName(final String name) {
return name.equalsIgnoreCase("takeUnitControl") || name.equalsIgnoreCase("giveUnitControl");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,7 @@ public Optional<IAttachment> newAttachment(
final GameData gameData) {
checkNotNull(typeName);

final String normalizedTypeName =
typeName.replaceAll("^games\\.strategy\\.triplea\\.attachments\\.", "");
final String normalizedTypeName = typeName.replaceAll("^.*\\.", "");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this change is working as intended.
The reason why the code was like this in the first place is because there is some ambiguity regarding the naming of attachments. "twoIfBySea" has a different namespace unfortunately.
See https://forums.triplea-game.org/topic/595/short-attachment-names for more information

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need to test and fix that if needed. Having an 'or' regex is not really ideal, but we could do it. IMO the java-class designation ideally would be completely removed, but we'll need to support it going forward. Perhaps we could do just straight up hardcoded mappings. We still have reflection going on to create classes by name, an explicit mapping with explicit construction code is I think where I'd like to go (and then couple that with updating the XML so that java classes are inferred on the backend and no longer needed in the XML).

final @Nullable AttachmentFactory attachmentFactory =
attachmentFactoriesByTypeName.get(normalizedTypeName);
if (attachmentFactory != null) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package games.strategy.triplea.attachments;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableMap;
import games.strategy.engine.data.Attachable;
import games.strategy.engine.data.GameData;
Expand All @@ -18,6 +19,7 @@
import java.util.List;
import java.util.Map;
import java.util.Set;
import org.triplea.java.RenameOnNextMajorRelease;
import org.triplea.java.collections.CollectionUtils;

/** The Purpose of this class is to hold shared and simple methods used by RulesAttachment. */
Expand All @@ -34,6 +36,7 @@ public abstract class AbstractRulesAttachment extends AbstractConditionsAttachme
// only matters for objectiveValue, does not affect the condition
protected int uses = -1;
// condition for what turn it is
@RenameOnNextMajorRelease(newName = "rounds")
protected Map<Integer, Integer> turns = null;
// for on/off conditions
protected boolean switched = true;
Expand Down Expand Up @@ -206,19 +209,16 @@ private void setRounds(final String rounds) throws GameParseException {
}
}

private void setTurns(final String turns) throws GameParseException {
setRounds(turns);
}

private void setTurns(final Map<Integer, Integer> value) {
private void setRounds(final Map<Integer, Integer> value) {
turns = value;
}

private Map<Integer, Integer> getTurns() {
@VisibleForTesting
public Map<Integer, Integer> getRounds() {
return turns;
}

private void resetTurns() {
private void resetRounds() {
turns = null;
}

Expand Down Expand Up @@ -433,8 +433,9 @@ public Map<String, MutableProperty<?>> getPropertyMap() {
"uses",
MutableProperty.of(this::setUses, this::setUses, this::getUses, this::resetUses))
.put(
"turns",
MutableProperty.of(this::setTurns, this::setTurns, this::getTurns, this::resetTurns))
"rounds",
MutableProperty.of(
this::setRounds, this::setRounds, this::getRounds, this::resetRounds))
.put(
"switch",
MutableProperty.of(
Expand All @@ -443,7 +444,6 @@ public Map<String, MutableProperty<?>> getPropertyMap() {
"gameProperty",
MutableProperty.ofString(
this::setGameProperty, this::getGameProperty, this::resetGameProperty))
.put("rounds", MutableProperty.ofWriteOnlyString(this::setRounds))
.build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -908,11 +908,13 @@ private void resetPlayers() {
players = new ArrayList<>();
}

private void setPlayerAttachmentName(final String name) throws GameParseException {
if (name == null) {
playerAttachmentName = null;
private void setPlayerAttachmentName(final String playerAttachmentName)
throws GameParseException {
if (playerAttachmentName == null) {
this.playerAttachmentName = null;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return;
}
final String name = playerAttachmentName.replaceAll("ttatch", "ttach");
final String[] s = splitOnColon(name);
if (s.length != 2) {
throw new GameParseException(
Expand Down Expand Up @@ -961,7 +963,7 @@ private void setPlayerAttachmentName(final String name) throws GameParseExceptio
&& !s[0].startsWith(Constants.USERACTION_ATTACHMENT_PREFIX)) {
throw new GameParseException("attachment incorrectly named:" + s[0] + thisErrorMsg());
}
playerAttachmentName = Tuple.of(s[1], s[0]);
this.playerAttachmentName = Tuple.of(s[1], s[0]);
}

private void setPlayerAttachmentName(final Tuple<String, String> value) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3690,12 +3690,6 @@ public Map<String, MutableProperty<?>> getPropertyMap() {
.put(
"isAir",
MutableProperty.of(this::setIsAir, this::setIsAir, this::getIsAir, this::resetIsAir))
.put(
"isMechanized", // kept for map compatibility; remove upon next map-incompatible release
MutableProperty.ofWriteOnlyString(s -> {}))
.put(
"isParatroop", // kept for map compatibility; remove upon next map-incompatible release
MutableProperty.ofWriteOnlyString(s -> {}))
.put(
"isSea",
MutableProperty.of(this::setIsSea, this::setIsSea, this::getIsSea, this::resetIsSea))
Expand Down Expand Up @@ -3943,13 +3937,6 @@ public Map<String, MutableProperty<?>> getPropertyMap() {
this::setIsAirTransportable,
this::getIsAirTransportable,
this::resetIsAirTransportable))
.put(
"isInfantry", // kept for map compatibility; remove upon next map-incompatible release
MutableProperty.of(
this::setIsLandTransportable,
this::setIsLandTransportable,
this::getIsLandTransportable,
this::resetIsLandTransportable))
.put(
"isLandTransport",
MutableProperty.of(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -500,8 +500,10 @@ public Color getPlayerColor(final String playerName) {

/** returns the color for impassable territories. */
public Color impassableColor() {
// just use getPlayerColor, since it parses the properties
return getPlayerColor(Constants.PLAYER_NAME_IMPASSABLE);
return Optional.ofNullable(
getColorProperty(PROPERTY_COLOR_PREFIX + Constants.PLAYER_NAME_IMPASSABLE))
.or(() -> Optional.ofNullable(getColorProperty(PROPERTY_COLOR_PREFIX + "Impassible")))
.orElseGet(defaultColors::nextColor);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Off-Topic: I don't think we have a nice error message for "too few default colors" yet, so that might be something to consider here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, just saw #7637

}

/**
Expand Down
Loading