Adding Human and NightElf Build #5
Conversation
to constructor in CUnitType.java
function in CUnit.java and added call to it in update()
adding food cost changes wisp death upon destruction during construction wisp return upon canceling construction
|
Hi! I'm part of the way through reviewing this and I like the direction it's going. But I noticed that this pull request is targeting main instead of experimental, which means that later if we merge experimental into main then there will be conflicts with the contents of this branch. In particular, we are now in a weird state where this pull request contains a better human powerbuild solution, but also a hit point regeneration solution that if I'm not mistaken doesn't honor blight/night, whereas the "experimental" branch powerbuild is worse (does not appropriately charge gold/lumber) but has a hit point regeneration system that does honor blight/night. So, I have been a tad slow to merge this pull request because (although it's kind of on me for branching off with the experimental branch) I was avoiding dealing with the inevitable merge conflicts. Do you have a preference on this? Are you planning to wait until I resolve this conflict somewhere in my workflow, or are you interested in resolving it on your end and updating the pull request to target the "experimental" branch? |
| @@ -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"); | |||
There was a problem hiding this comment.
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:
(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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Also, to what exactly should I rebase? (Just so that I don’t mess things up even further)
| @@ -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); | |||
There was a problem hiding this comment.
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.
| * (If something with this is off it's probably my fault) | ||
| */ | ||
| private void regenerateHitPoints(){ | ||
| if(this.life<this.getUnitType().getMaxLife()) { |
There was a problem hiding this comment.
In this case, we want to check if this.life is less than this.maximumLife actually. For heroes or upgraded units who benefit from upgrades like Improved Masonry, the difference will become significant.
| */ | ||
| private void regenerateHitPoints(){ | ||
| if(this.life<this.getUnitType().getMaxLife()) { | ||
| this.life += (this.getUnitType().getLifeRegeneration() * WarsmashConstants.SIMULATION_STEP_TIME); |
There was a problem hiding this comment.
Likewise here for the regeneration rate; strength will increase the rate so we need to read it from the unit and not his type.
Caveat to that, unlike the "maximumLife" issue, regeneration rate as an member variable of "CUnit" probably did not exist at the time that you authored this change so it's less a bug not to use it (whereas we can see that at the time this change was authored, "CUnit.maximumLife" member variable did already exist)
| popoutWorker(game); | ||
| } | ||
|
|
||
| //create Blight here (MFROMAZ) |
There was a problem hiding this comment.
Are you creating blight here? That would be cool, but I get the impression this might only be a dangling comment. I think that when your commit was authored, I did not have blight in the engine. However, at this point the experimental branch implements the ability code 'Abli' with the outward growth at high speed when a building finishes construction. (As an example, see this video at time 1:26)
| float newLifeValue = targetWidget.getLife() + | ||
| ((WarsmashConstants.SIMULATION_STEP_TIME / (targetUnit.getUnitType().getBuildTime()*this.ability.getRepairTimeRatio())) | ||
| * (targetUnit.getMaxLife())); | ||
| float costs_gold = ((WarsmashConstants.SIMULATION_STEP_TIME / (targetUnit.getUnitType().getBuildTime()*this.ability.getRepairTimeRatio())) |
There was a problem hiding this comment.
Everywhere else in the codebase I used Java variable naming convention -- i.e. camel case -- instead of underscores. It's fairly pedantic and if this was my only concern I'd probably still merge this anyway, but if you find yourself reviewing this it could be nice for consistency to use costsGold instead of costs_gold.
| targetWidget.setLife(simulation, newLifeValue); | ||
| if(done) { | ||
| //progress construction here (MFROMAZ) | ||
| if(targetWidget.getClass()==CUnit.class) { |
There was a problem hiding this comment.
So far in Warsmash I have preferred to use targetWidget instanceof CUnit instead of targetWidget.getClass() == CUnit.class, and there is actually an important difference in a future codebase even though the current code has no difference. Basically, if someone decided to do the (rather silly) idea of subclassing CUnit to make CUnitWard or something, then (new CUnitWard(...)) instanceof CUnit would return true but (new CUnitWard(...).getClass() == CUnit.class would return false.
This is another fairly pedantic thing and not a substantial issue. If we wanted to be super pedantic about code design, in theory I think it's better to never do either of these and instead use the ability target visitor that I set up, i.e. target.visit(AbilityTargetVisitor<Boolean>() {...}) and implement some AbilityTargetVisitor who returns true for units. The instanceof case, although theoretically better than class equality, can be stupid if we copy CUnit to make some other type like maybe CItem if we did not already have that. The visitor would provide a compile error forcing the human to reconsider what to do with a CItem in this case where it previously was not considered, while the instanceof would silently ignore the CItem as a non-unit.
Just a brainstorm on this one. Again, maybe not super important. For the sake of brevity/laziness, as I'm sure you've seen already, I contradicted this knowledge and used instanceof CUnit a lot already in Warsmash. The probability of adding additional subclasses of CWidget is extraordinarily low in this project.
| Path02="..\..\War3xlocal.mpq" | ||
| Type03=MPQ | ||
| Path03="..\..\War3Patch.mpq" | ||
| Type04=MPQ |
There was a problem hiding this comment.
Instead of MPQ for the resources, it would probably be better if the github used
Type04=Folder
Path04="resources/"I figure is critically important that the resources folder does not accidentally include any Blizzard assets for legal reasons -- same reason this entire project cannot include any Blizzard assets -- and seeing we did this correctly is easier to verify with a folder tracked by git than if we include this file as a packaged binary.
| if(!(this.getConstuctionProcessType()!=null&&this.getConstuctionProcessType()==ConstructionFlag.CONSUME_WORKER)){ | ||
| popoutWorker(simulation); | ||
| }else{ | ||
| if(workerInside!=null) { |
There was a problem hiding this comment.
If we are already going to check again whether the worker inside is null, maybe we should have ConstructionFlag.NO_WORKER for the Undead instead? This would purely be a pedantic change; it's just a suggestion and it's not necessary. It's probably beneficial to null check the workerInside for possible crazy custom maps anyways.
| return unit.pollNextOrderBehavior(simulation); | ||
| } | ||
| boolean done = (newLifeValue >= targetWidget.getMaxLife()); | ||
| boolean done = (newLifeValue >= ((CUnit) targetWidget).getUnitType().getMaxLife()); |
There was a problem hiding this comment.
As a general rule, we should probably avoid getUnitType().getMaxLife() except during unit creation and when upgrading a unit into a different type, simply because of the possibility of heroes with life added by strength, or buildings with the Improved Masonry upgrade, etc.
|
Sorry for taking so long to reply. Life is busy right now. Thanks for the feedback. I’ve changed the pull request to target experimental. I’d be happy to resolve these issues, but it might take a few weeks for me to get around to everything, so don’t let me delay you, if you want to make quick progress or something. Maybe let me know if you fix something in case I take too long. Otherwise, I’ll reply to the individual comments once I get around to the concrete things you ask me to do there. |
Implementation of Human and NightElf construction with power build, repair, wisp consumption and costs