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

Adding Human and NightElf Build #5

Open
wants to merge 6 commits into
base: experimental
Choose a base branch
from
Open
Changes from 1 commit
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
Expand Up @@ -33,7 +33,6 @@
import com.etheller.warsmash.viewer5.handlers.w3x.simulation.abilities.hero.CPrimaryAttribute;
import com.etheller.warsmash.viewer5.handlers.w3x.simulation.abilities.queue.CAbilityQueue;
import com.etheller.warsmash.viewer5.handlers.w3x.simulation.abilities.queue.CAbilityRally;
import com.etheller.warsmash.viewer5.handlers.w3x.simulation.abilities.queue.CAbilityReviveHero;
import com.etheller.warsmash.viewer5.handlers.w3x.simulation.combat.CAttackType;
import com.etheller.warsmash.viewer5.handlers.w3x.simulation.combat.CDefenseType;
import com.etheller.warsmash.viewer5.handlers.w3x.simulation.combat.CTargetType;
Expand All @@ -52,6 +51,7 @@ public class CUnitData {
private static final War3ID MANA_INITIAL_AMOUNT = War3ID.fromString("umpi");
private static final War3ID MANA_MAXIMUM = War3ID.fromString("umpm");
private static final War3ID HIT_POINT_MAXIMUM = War3ID.fromString("uhpm");
private static final War3ID HIT_POINT_REGENERATION = War3ID.fromString("uhpr");
private static final War3ID MOVEMENT_SPEED_BASE = War3ID.fromString("umvs");
private static final War3ID PROPULSION_WINDOW = War3ID.fromString("uprw");
private static final War3ID TURN_RATE = War3ID.fromString("umvr");
Expand Down Expand Up @@ -144,7 +144,6 @@ public class CUnitData {
private static final War3ID STRUCTURES_BUILT = War3ID.fromString("ubui");
private static final War3ID UNITS_TRAINED = War3ID.fromString("utra");
private static final War3ID RESEARCHES_AVAILABLE = War3ID.fromString("ures");
private static final War3ID REVIVES_HEROES = War3ID.fromString("urev");
Copy link
Owner

@Retera Retera Apr 27, 2022

Choose a reason for hiding this comment

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

Because the earliest commits of your pull request began by committing a change into git to eliminate the work that I had been doing at the same time (although surely this was simply a git typo) the result is that even after you attempted to reconcile it, use of automatic tools to review the author of particular lines of code will misattribute this, giving you credit for this line of code that you "added back" even though you were not the original author of the line of code:
image
(screenshot is from an inspection of your fork in its current state)

Would you be willing to rebase the changes so to avoid this type of misattribution? I know this is a fairly pedantic request from me but I was burned by this a few years ago by working on a project with people who were substantially less interested, and ultimately I abandoned the project after they did this type of thing a lot. The reason I eventually discontinued work on the project was essentially because of concerns that I would no longer be considered the author of the game engine that the other author had contributed almost nothing legitimate to at that time. In the case of an open source project like Warsmash that same issue does not apply. However, accurate git attributions for the individual lines of code can be valuable when hunting for bugs or interested in the history of a particular feature.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, no problem. I understand completely. I’m kind of not so great with git but I’ll figure it out. I’ll let you know, when I got around to it

Copy link
Author

Choose a reason for hiding this comment

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

Also, to what exactly should I rebase? (Just so that I don’t mess things up even further)

private static final War3ID UNIT_RACE = War3ID.fromString("urac");

private static final War3ID REQUIRES = War3ID.fromString("ureq");
Expand Down Expand Up @@ -245,10 +244,7 @@ public CUnit create(final CSimulation simulation, final int playerIndex, final W
if (!unitsTrained.isEmpty() || !researchesAvailable.isEmpty()) {
unit.add(simulation, new CAbilityQueue(handleIdAllocator.createId(), unitsTrained, researchesAvailable));
}
if (unitTypeInstance.isRevivesHeroes()) {
unit.add(simulation, new CAbilityReviveHero(handleIdAllocator.createId()));
}
if (!unitsTrained.isEmpty() || unitTypeInstance.isRevivesHeroes()) {
if (!unitsTrained.isEmpty()) {
unit.add(simulation, new CAbilityRally(handleIdAllocator.createId()));
}
if (unitTypeInstance.isHero()) {
Expand All @@ -274,6 +270,7 @@ private CUnitType getUnitTypeInstance(final War3ID typeId, final BufferedImage b
if (unitTypeInstance == null) {
final String legacyName = getLegacyName(unitType);
final int life = unitType.getFieldAsInteger(HIT_POINT_MAXIMUM, 0);
final float lifeRegeneration = unitType.getFieldAsFloat(HIT_POINT_REGENERATION,0);
Copy link
Owner

Choose a reason for hiding this comment

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

On the experimental branch changes that we will eventually want to reconcile this with, when I was implementing a similar feature to this, I instinctively shorthanded the name and called the variable "lifeRegen" instead of "lifeRegeneration". For whichever of us ends up resolving the merge conflicts, when in doubt I tend to prefer "lifeRegeneration" if I spend time to think about it, because in general shorthand does not improve the performance of code in 2022 so the only reason to use it would be to type more quickly, and any good IDE autosuggest should drop-in "lifeRegeneration" as soon as the user types lR<auto insert key> let alone "lifeRegen", so there is actually no justifiable argument that I can think of for me to have used "lifeRegen" in 2022 other than the fact that it did not occur to me to care.

So anyway, my thought here is to say good job and I think yours is better than the one I was working on as far as the naming convention. But when we resolve that, the "experimental" branch also has "manaRegen" which is similar and should most likely be renamed to "manaRegeneration" for consistency.

final int manaInitial = unitType.getFieldAsInteger(MANA_INITIAL_AMOUNT, 0);
final int manaMaximum = unitType.getFieldAsInteger(MANA_MAXIMUM, 0);
final int speed = unitType.getFieldAsInteger(MOVEMENT_SPEED_BASE, 0);
Expand Down Expand Up @@ -455,8 +452,6 @@ private CUnitType getUnitTypeInstance(final War3ID typeId, final BufferedImage b
final int foodUsed = unitType.getFieldAsInteger(FOOD_USED, 0);
final int foodMade = unitType.getFieldAsInteger(FOOD_MADE, 0);

final boolean revivesHeroes = unitType.getFieldAsBoolean(REVIVES_HEROES, 0);

final String unitsTrainedString = unitType.getFieldAsString(UNITS_TRAINED, 0);
final String[] unitsTrainedStringItems = unitsTrainedString.trim().split(",");
final List<War3ID> unitsTrained = new ArrayList<>();
Expand Down Expand Up @@ -538,14 +533,14 @@ else if (requirementsLevelsStringItems.length > 0) {

final List<String> heroProperNames = Arrays.asList(properNames.split(","));

unitTypeInstance = new CUnitType(unitName, legacyName, typeId, life, manaInitial, manaMaximum, speed,
unitTypeInstance = new CUnitType(unitName, legacyName, typeId, life, lifeRegeneration, manaInitial, manaMaximum, speed,
defense, abilityList, isBldg, movementType, moveHeight, collisionSize, classifications, attacks,
armorType, raise, decay, defenseType, impactZ, buildingPathingPixelMap, deathTime, targetedAs,
acquisitionRange, minimumAttackRange, structuresBuilt, unitsTrained, researchesAvailable, unitRace,
goldCost, lumberCost, foodUsed, foodMade, buildTime, preventedPathingTypes, requiredPathingTypes,
propWindow, turnRate, requirements, unitLevel, hero, strength, strPlus, agility, agiPlus,
intelligence, intPlus, primaryAttribute, heroAbilityList, heroProperNames, properNamesCount,
canFlee, priority, revivesHeroes);
canFlee, priority);
this.unitIdToUnitType.put(typeId, unitTypeInstance);
this.jassLegacyNameToUnitId.put(legacyName, typeId);
}
Expand Down