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

Factions/grey knights #13

Merged
merged 9 commits into from
Sep 7, 2024
Merged

Factions/grey knights #13

merged 9 commits into from
Sep 7, 2024

Conversation

gbytz
Copy link
Contributor

@gbytz gbytz commented Sep 5, 2024

No description provided.

@gbytz gbytz marked this pull request as draft September 5, 2024 18:00
@gbytz gbytz marked this pull request as ready for review September 5, 2024 18:01
@dixhuit
Copy link
Contributor

dixhuit commented Sep 5, 2024

A PR! Nice one :)

Not my repo but I noticed that a few units come up as "Unit 1" when you try to select a unit from an advanced profile:

image

Tidying these up will help (below image). If there's only 1 unit in a profile, just rename the unit to match the profile name. Is that the convention @korzxd ?

image

@gbytz
Copy link
Contributor Author

gbytz commented Sep 6, 2024

@dixhuit Interesting. 🤔 Honestly I didn't pay much attention to the Advance profiles feature and just loaded everything using the Basic profile view. Could it be that those profiles that have the proper name on them are the ones that I happen to just open and save from the Advance profile view? In any case I'll check it. Thanks!

@gbytz
Copy link
Contributor Author

gbytz commented Sep 6, 2024

@dixhuit It seems that if a profile is a clone it doesn't automatically updates the names in the Advance profile units. For example my NDK is a clone from my GMNDK and when I checked its Advanced profile it looked like this.
image
Not sure if consider it a bug because is a clone and I didn't bother to check the Advance profile when I make it but I would've expected that changing the name from the Basic profile would update the one in the Advance. 🤔

@gbytz gbytz force-pushed the factions/grey-knights branch from ba57f2c to cb9a776 Compare September 6, 2024 05:54
@gbytz
Copy link
Contributor Author

gbytz commented Sep 6, 2024

I had to force push to prevent leaking my personal information so you'll have to re-checkout this branch. Sorry for the inconveniences.

@dixhuit
Copy link
Contributor

dixhuit commented Sep 6, 2024

Latest version fixes the unit name issue, nice one.

Regarding the cloning/naming issue raised, that's a great point and it's not clear to me how best to handle this beyond what it already does. I will give it some thought, thanks for pointing it out.

@korzxd
Copy link
Owner

korzxd commented Sep 6, 2024

Thanks @gbytz for the submission this is a great amount of work and i really appreciate the contribution, also thanks to @dixhuit for reviewing the PR and providing suggestions 😄.

In response to:

Tidying these up will help (below image). If there's only 1 unit in a profile, just rename the unit to match the profile name. Is that the convention @korzxd ?

Yeah that's exactly how would i name them. Here's an example of how i would name mixed model types as well.
image

@gbytz I have had a quick look at the Export this morning and so far it looks spot! The formatting and detail in the readme is a welcome sight as well, good job 👍 Thank you!

I will properly review this later on this evening and get this PR merged.

@gbytz
Copy link
Contributor Author

gbytz commented Sep 6, 2024

@korzxd 🫡 I'm just trying to give back to the community. I noticed that I left the "x5" in the model type name for some units. I'll fix those and bump the version to 1.0 so it would be ready for merge once you finish reviewing.

@korzxd
Copy link
Owner

korzxd commented Sep 6, 2024

Okay i've had a look at it and it's near perfect, there are just a few things i spotted that have the wrong values along with a couple other suggestions such as abilities that generate mortals. This is a great PR that will help grow the repository and i'm very grateful for the huge amount of work you've put into this! 😄

Below is the list of everything i picked up on, i can either address these at a later date or you can it's entirely up to you but i can imagine after putting the work in you would probably want to finalize the last changes.

Changes

  • Brother-Captain:
    • Add the (if attack is psychic) condition to both profile abilities.
  • Brother-Captain Stern :
    • Add the (melee only) condition to the profile ability.
  • Brotherhood Librarian:
    • Change AP Characteristic on the Nemesis force weapon to -1.
    • This is just a suggestion but you could change the Vortex ability to use the Attack Condition Attack step roll along with the Mortal Wounds effect to generate mortals, this would also simulate the average chance of generating the 2d6 Mortal wounds over the 2d3 Mortal wounds. Here's an example of what i would do: Brotherhood Librarian.txt
  • Brotherhood Techmarine:
    • Change Wounds Characteristic to 4
    • The EXTRA ATTACKS ability has just been released in the latest UnitCrunch version and can be added to the weapons Servo-arms & Plasma cutter. This weapon ability can be found by creating a new weapon ability and selecting the Special effect, then pick Extra Attacks from the combo box.
  • Grand Master Voldus:
    • As with the Brotherhood Librarian you could condense the two Hammer Aflame weapons into one and add Keyword only conditions to match the effect. Here's an example: Grand Master Voldus.txt
  • Grey Knights Stormraven Gunship:
    • Remove the weapons: Storm bolter and Multi-melta.
  • Purgation Squad x5:
    • Change Set BS to 4 (ranged only) (if attacker) to Only hit on a 4+ (unmodified) (irrespective of attack modifiers) (if attacker). If we set the BS to 4 along with the -1 to hit roll the weapons in the simulation will actually only hit on a 5+, by using an Only hit on a # ability we can properly simulate the Indirect Fire condition that they always fails on an Unmodified hit roll of 1-3. Here's a modified Profile to show you the ability: Purgation Squad x5.txt
    • Make the profile abilities Shared.
  • Grey Knights Thunderhawk Gunship:
    • Change Wounds Characteristic to 30

Adjust the Readme to reflect the above changes.

@korzxd
Copy link
Owner

korzxd commented Sep 6, 2024

You also mentioned advanced profiles in your previous comments but i could not see a single advanced profile in your any of your exports including the latest one, i don't know if this is an error with the import process my side or something has happened to the export. @dixhuit would it be possible for you to clarify whether you see any advanced profiles that were in the previous releases in this latest export?

@dixhuit
Copy link
Contributor

dixhuit commented Sep 6, 2024

@korzxd Looking over the profile names I don't see the need for any of them to be advanced profiles. They're all single unit profiles that don't require >1 model type, unless I'm missing something?

@dixhuit
Copy link
Contributor

dixhuit commented Sep 6, 2024

One other thing I spotted is that all weapons appear to be enabled by default for all profiles (well, the ones I've checked). Not sure if this is convention or not so I could be pointing out nothing here. Would it not make more sense to have a "legal" combination of weapons enabled by default? I don't play GK though - for all I know this is legal(!), just seemed like a lot at once.

image

@gbytz
Copy link
Contributor Author

gbytz commented Sep 7, 2024

@korzxd I’ll fix all those and report back soon.

Brother-Captain:
    Add the `(if attack is psychic)` condition to both
profile abilities.

Brother-Captain Stern:
    Add the `(melee only)` condition to the profile
ability.

Brotherhood Librarian:
    Change AP Characteristic on the Nemesis force weapon to -1.
    Condense `Vortex of Doom` weapon profiles into one.

Brotherhood Techmarine:
    Change Wounds Characteristic to 4
    Add `EXTRA ATTACKS` ability to `Servo-arms` and `Plasma-cutter` weapons

Grand Master Voldus:
    Condense `Hammer Aflame` weapon profiles into one.

Grey Knights Stormraven Gunship:
    Remove `Storm bolter` and `Multi-melta weapons`.

Purgation Squad x5:
    Change `Set BS to 4 (ranged only) (if attacker)` to `Only hit on a
    4+ (unmodified) (irrespective of attack modifiers) (if attacker)`.

Grey Knights Thunderhawk Gunship:
    Change Wounds Characteristic to 30
@korzxd
Copy link
Owner

korzxd commented Sep 7, 2024

@korzxd Looking over the profile names I don't see the need for any of them to be advanced profiles. They're all single unit profiles that don't require >1 model type, unless I'm missing something?

Sorry I must've been mistaken, I was assuming there were some in the export just from your earlier comments about advanced profile names along with this commit mentioning advanced profile names cb9a776

Disregard my last comment if that was the case

@gbytz
Copy link
Contributor Author

gbytz commented Sep 7, 2024

I made all the requested changes, some comments below:

  • Brotherhood Librarian:

I applied your patch after double checking it. 👍

  • Grand Master Voldus:

I was going to apply your patch but I noticed the BS there was 6+ and the keywords referenced there were INFANTRY. So I set the BS to 2+ and change the keywords to DAEMON.

  • Grey Knights Thunderhawk Gunship:

On top of changing the Wounds characteristic to 30 I changed the Thunderhawk Cluster Bombs weapon name to Thunderhawk Cluster Bombs - (Ability) and removed the redundant Always wound ability.

Regarding Advanced profiles: there are no profiles that actually need to be Advanced because as @dixhuit mentioned all are single unit profiles. My plan is to add profiles for Leader + Bodyguard units and bigger sized units in a 1.1 version after this gets in.

@dixhuit Regarding all weapon profiles being enabled: I do not see away to set those enabled or disable in the Edit profile view, only in the Crunch view but, in case the setting in the Crunch view are somehow saved in the exported file, I went over all the profiles, disable all the weapons and re-save them.

On the topic of Wargear Options, I added multiple weapon profiles of the same weapon but with different count numbers to be able to quickly change Wargear combinations in the Crunch view without having to edit the profile. My rationale behind this is that:

  1. I change Wargear more often than I actually want to change Weapon counts per unit.
  2. I find it less error prone in the sense that I do not need to remember to edit the profile back to the original number.

The drawbacks are:

  1. Crunch view might get a bit crowded. But that is not a problem (for me at least)
  2. It is kind of a maintenance nightmare if the weapon has to be changed. But I think that will not happen often.

Given the limit on the amount of weapon profiles per unit, is not always possible to have enough weapon profiles to make all the possible Wargear combinations for a unit. But that is not a problem in most of the cases and in those were it is, I choose to add enough weapon profiles to be able to make the most common (optimized) or sensible Wargear combinations.

@dixhuit
Copy link
Contributor

dixhuit commented Sep 7, 2024

@dixhuit Regarding all weapon profiles being enabled: I do not see away to set those enabled or disable in the Edit profile view, only in the Crunch view

Apologies. You are correct and I am confusing reality with a task on my to-do list that I've yet to implement! What's worse is I've not even created a GitHub issue for this task that I could link to (the to-do list has had many forms over the years, not everything has been migrated to GitHub issues yet). I'll do that now:

dixhuit/unitcrunch-issue-tracker#115

In the meantime, I do believe that the weapons selected state is saved with the profile when interacted with as a selected attacker on the crunch page.

@dixhuit
Copy link
Contributor

dixhuit commented Sep 7, 2024

On the topic of Wargear Options, I added multiple weapon profiles of the same weapon but with different count numbers to be able to quickly change Wargear combinations in the Crunch view without having to edit the profile.

Related planned feature that will hopefully avoid having to do this in future: dixhuit/unitcrunch-issue-tracker#116

@korzxd korzxd merged commit fe8f0fc into korzxd:main Sep 7, 2024
@korzxd
Copy link
Owner

korzxd commented Sep 7, 2024

Thanks for finalizing the changes it all looks good to me! I'll get this merged and thanks again for the contribution :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants