Feature - Black Ice terrain modification #4521
Conversation
pheonixstorm
added
(RFE) Enhancement
|
Is this a draft? If so, maybe convert the PR to draft for now. |
| // inferno bombs, 4: inferno IV | ||
| /** The SMOKE terrain type includes laser-inhibiting smoke and chaff from chaff pods (TO:AUE p111). */ | ||
| public static final int SMOKE = 20; // 1: light smoke 2: heavy smoke 3:light | ||
| public static final int SMOKE = 21; // 1: light smoke 2: heavy smoke 3:light |
There was a problem hiding this comment.
Let's definitely not change these constants, as it'll wreck every map that uses the terrain types whose value was changed.
There was a problem hiding this comment.
The maps themselves do not use this number. The entry in the map itself reads smoke:1 - smoke:4. I did find that the game saves do use 20 rather than smoke:1 etc. This won't wreck maps, but it will a save.
From a test board file I get: hex 0101 0 "smoke:1" ""
From a save game file I get:
<megamek.common.Terrain id="2135">
20
1
false
0
0
</megamek.common.Terrain>
I'm not sure if this should go to the bottom, but that would put it out of place based on the ordering of the constants. Terrain, mods, building etc. Might want to discuss this with the rest of the team as if I place this below smoke there is no telling if this will break other save games while placing it at the bottom of the list means that the constants are longer organized by terrain types. But since you can't play a save from one version to another that renders this entire review point moot. Unless that changed of course.
There was a problem hiding this comment.
We don't particularly care about backwards compatibility for save games. You can always leave it in the same place in the code but have the number be different. Feel free to ask in discord, though, as long as it doesn't break maps I don't mind it either way..
There was a problem hiding this comment.
Was this covered on Discord? Trying to find loop closure on the question. This seems like a change that could have unintended consequences.
There was a problem hiding this comment.
@pheonixstorm Would you mind actually changing this and placing BLACK_ICE below the other terrains (I think we care the least about the ordering of the terrains) and give it the 55? It is likely that it would only break savegames, so it may be just a matter of principle, but it's certainly good form not to change those constants without any real need. Also, leaving BLACK_ICE where it is and giving it the 55 invites the next terrain accidentally also using 55. I actually found such a thing awhile ago in another class that used the 71 for two different equipments.
There was a problem hiding this comment.
@SJuliez the ordering is based off of the sections in Tac Ops for the terrain types. I don't mind placing it at the bottom of the Terrain Modifications section but not at the very bottom of the list. While I am making that change I can also add in the constants for the missing terrain types and modifications as well, something I had planned on doing when I started the Hazardous Liquid Pools.
As for savegames, they don't carry over between versions so at worst it affects the nightlies. I don't think those are affected moving from one nightly to another.
There was a problem hiding this comment.
I feel like if three people call this out as a potential problem, it might be worth taking a different approach.
|
Not to self: Find where the terrain names are stored for the map editor. Will need to add Black Ice entry for it. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
pheonixstorm
removed
the
In Development (Draft)
|
@NickAragua this is ready for another look |
| // inferno bombs, 4: inferno IV | ||
| /** The SMOKE terrain type includes laser-inhibiting smoke and chaff from chaff pods (TO:AUE p111). */ | ||
| public static final int SMOKE = 20; // 1: light smoke 2: heavy smoke 3:light | ||
| public static final int SMOKE = 21; // 1: light smoke 2: heavy smoke 3:light |
There was a problem hiding this comment.
Was this covered on Discord? Trying to find loop closure on the question. This seems like a change that could have unintended consequences.
| // inferno bombs, 4: inferno IV | ||
| /** The SMOKE terrain type includes laser-inhibiting smoke and chaff from chaff pods (TO:AUE p111). */ | ||
| public static final int SMOKE = 20; // 1: light smoke 2: heavy smoke 3:light | ||
| public static final int SMOKE = 21; // 1: light smoke 2: heavy smoke 3:light |
There was a problem hiding this comment.
I feel like if three people call this out as a potential problem, it might be worth taking a different approach.
# Conflicts: # megamek/src/megamek/common/options/GameOptions.java # megamek/src/megamek/server/ServerHelper.java
# Conflicts: # megamek/src/megamek/common/options/GameOptions.java
SJuliez
added
the
For New Dev Cycle
Ice Storm includes Black Ice by default
Optional Black Ice in weather at or below -30
The Ice change per rules is at 5 or 6 on a 1d6, currently for testing it is set at 2-6 so the likelihood of NOT hitting black ice is slim.
This is nearly finished and put up to be picked apart.
Happy skidding!