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

Dunefolk rework #4195

Merged
merged 1 commit into from Aug 10, 2019

Conversation

@Vultraz
Copy link
Member

commented Jul 29, 2019

@Earth-Cake tells me this is feature-complete. Awaiting description of changes and list of people to credit.

@Vultraz Vultraz changed the title New DuneFolk changes by various people New Dunefolk changes by various people Jul 29, 2019

@@ -75,6 +75,19 @@ This ability will not cure an affected unit of poison, however, only delay its e
[/regenerate]
#enddef

#define ABILITY_SELF_HEAL

This comment has been minimized.

Copy link
@ProditorMagnus

ProditorMagnus Jul 29, 2019

Contributor

I would like macro name be closer to ability id.

This comment has been minimized.

Copy link
@Vultraz

Vultraz Jul 29, 2019

Author Member

Should the ID be the same as the Regen +8 ability, like how Healing +8 and +4 have the same healing ID?

This comment has been minimized.

Copy link
@FranPrin

FranPrin Jul 30, 2019

Contributor

We didn't know what was best to do for the macro name. We considered "LESSER_REGENERATES" such as is the trend with heal and extra heal. But weren't sure that was actually better.

Also strangely the id for heals were both the same (as vultraz has pointed out above) so we weren't sure if that was standard procedure or if it would be better to make a new id. Please feel free to commit any change you think is appropriate.

This comment has been minimized.

Copy link
@ProditorMagnus

ProditorMagnus Jul 30, 2019

Contributor

I believe the reason for same id is to not have them cumulative.

data/core/units.cfg Outdated Show resolved Hide resolved
number=3
damage=6
number=4
icon=attacks/mace.png

This comment has been minimized.

Copy link
@ProditorMagnus

ProditorMagnus Jul 29, 2019

Contributor

Name and icon match, so icon is not needed

@@ -1,18 +1,13 @@
#textdomain wesnoth-units
[unit_type]
# Below unit id may need changing to use underscores in unit ID only (not in displayed name) to fix bug #18117, as stated in former Dune Scorcher file

This comment has been minimized.

Copy link
@ProditorMagnus

ProditorMagnus Jul 29, 2019

Contributor

What is 18117?

This comment has been minimized.

Copy link
@Vultraz

Vultraz Jul 29, 2019

Author Member

Looks like the old GNA ID...

This comment has been minimized.

Copy link
@ProditorMagnus

ProditorMagnus Jul 29, 2019

Contributor

I could not find reference to it in git issues

@ProditorMagnus

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

These should cover main classes of issues here.

@Pentarctagon

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

Regarding the unit test failure, it looks like that is due to the units declared here.

Depending on the movetype of the units replacing them, this line may also need to be updated.

@Vultraz Vultraz changed the title New Dunefolk changes by various people Dunefolk rework Jul 30, 2019

@soliton-

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

Here are the instructions on how to generate the new list of numbers for the unit test.

### Change GENERATE_ANSWERS to 1 in the next line to generate the correct answers for the characterization test.
### The list of answers will appear in console when you run wesnoth, after debug: messages.
### Copy the lines to a text file ("foo") and use the following sed script to clean them,
###
### sed -n 's/^.*: \([ [:digit:]]*\) ,/\1/p' <foo | tr '\n' ','
###
### so you get a nice comma separated list. Then paste it in data/test/macros/pathfind_answers.cfg,
### and in this file change GENERATE_ANSWERS to 0 again.

@nemaara nemaara force-pushed the dunefolk_rework_merge branch 2 times, most recently from 7698c20 to 25e7055 Aug 1, 2019

@Earth-Cake
Copy link
Contributor

left a comment

It was naphta, not naphtha in default Dunefolk in Firetrooper description.

data/core/units/nagas/Slasher.cfg Show resolved Hide resolved
@ghype

This comment has been minimized.

Copy link

commented Aug 1, 2019

@Vultraz
Here are the patch notes & credits

Art:

Most of units were reworked - some more some less - but our goal was to make them fit for mainline all together. If any changes are needed to any unit, then we gladly execute those. There have been months worth of progress documented in the according art thread for this project.

Base Units
Extra Units

Balance:

The main reason why this project started in the first place. Here as well we have all the progress for balancing Dunefolk documented and all developers and users interested in this project and seemed to agree on the current state of balance. Thats the reason why we will not enlist all the changes here. If there are any problems with the balance, then we too are willing to provide alternatives that can be discussed either here or on forum.

Balance Thread

Alignment:

The balance of this faction was based on the newly proposed liminal alignment which works as +0/+0/+25/+0/+0/+25 instead of the old -25/-25/+0/-25/-25/+0

Weapon Portraits:

As Dunefolk features entirely new units, we found some weapon portraits that fits the attacks of certain units. The portrait - called „blade-curved“ is a franken but a convincing one. As the current blow pipe for UtbS is a franken as well, this new weapon portrait should good enough to be mainlined as well. If this request get’s denied, then we are free to provide alternatives as well. It just fits very well the theme of exotic Naga weapon craftsmanship without relying on the orcish theme. It used for the following two units:

Lv2 Naga Bladewhirler
Lv3 Naga Dervish

Descriptions:

New unit concepts require changes and new units require entirely new descriptions. We enlisted all problems and inconsistencies that resulted with the rework and provided either solutions for them or new descriptions. Yumi already started doing them but for those who want to see the initial conflicts, here you can find all the problems displayed:

Unit Descriptions

Female Dunefolk Names:

With the introduction of female units to this faction, we would require female names as well. Apparently this was already coded in mainline, but simple not used.

Future Plans - Animations

There are some recourses from past works that allow us to - more or less - easily animate most of Dunefolk units. The animations range from idle to attack-only, while other units will be fully animated. Some of the units that are harder to animate will first receive single attack/defend frames until animation concepts were agreed upon. Our goal is to make Dunefolk as alive as possible.

Future Plans - Theme, Sound FX

Rather unimportant to most people but I am interested to add some new sound fx that can start with different hit/death sounds or new weapon sounds for scimitars. Those are just example and I do not know what I will end up with. But I know that I will compose a Dunefolk Theme at some point. Using your feedback, I am sure I can manage to create something that fits the faction the best.

Future Plans - Wyvern Rider

This is the only unit is the only one that did not get touched or reworked. The wyvern on it self looks amazing an does not need any rework but the rider does not really fit thematically anymore, not to mention the weapon is too oversized and not the usual Wesnoth style. But I won’t touch this sprite until it actually will be needed, most likely - IF - there is ever going to be a mainline Dunefolk campaign.

Credits:

I don’t know how detailed you need this to be but here all contributors.
ghype, Hejnewar, Krogen, The_Gnat, Lordlewis, EarthCake, Tom_Of_Wesnoth

It is however important to mention that the amount of contributions of each varies a lot in different categories. I will organise it a bit and you make use of this info how ever you want. The names are in order of the amount of work put in.

Art: ghype, The_Gnat, Lordlewis
Balance: Hejnewar, Krogen, ghype, The_Gnat
Descriptions: Tom_Of_Wesnoth, Hejnewar

Earthcake joined us in the end and helped us finishing this project by cleaning/finishing the codes and doing all the git stuff together with The_Gnat. Although none of his work can be categorised in one of the three categories, he still deserves to be credit in one way or another.

Thanks for your time and efforts.

@ghype

This comment has been minimized.

Copy link

commented Aug 1, 2019

This is the last things that will have to happen and other things to consider. I was the last one doing the double and tripple checks and after this, it indeed should be "feature complete".

To fix Code:

Lv1 Naga Slasher from icon=attacks/moonglaive.png to icon=attacks/dagger-curved.png
Lv2 Naga Bladewhirler from icon=attacks/moonglaive.png to icon=attacks/blade-curved.png
Lv3 Naga Dervish from icon=attacks/moonglaive.png to icon=attacks/blade-curved.png

Lv 2 Naga Ringcaster from icon=attacks/moonglaive.png to icon=attacks/chakram.png
Lv 3 Naga Zephyr from icon=attacks/moonglaive.png to icon=attacks/chakram.png

Are Dunefolk humans?

We certainly do know that they are, biggest give away are their appearance and the fact they share the +20% arcane resistance. But in our months of development, we encountered many players who didn’t knew wether they are or are not humans. I figured, for the sake of consistency, the image files could be stored like /images/units/human-dune or human-dunefolk. The unit files too would be put together with the other human and they would receive the a tag just like the loyalists or magicians which then could be „Dune_“. The folder then would accumulate many units but it still would be organised due to the tags.
I can see why some would rather not touch the directories for units/images but with VSC paths for all images can be changed with a single command and there is only one single map that uses Dunefolk as recourses. Logistically, its not that much of an effort and its worth considering for the sake of consistency.

Removal Of Unused Files:

Some units didn’t made the cut in the rework. Those units are Lv1 Piercer, Lv0 Falcon and Lv1 Elder Falcon. The image and unit file for the Lv1 Piercer should be removed entirely but it is considerable to move the Falcon files from the Dunefolk to the monster directories as it still can be used as an auxiliary unit. I have plans to even animate this unit line though I will do Dunefolk’s base units first.

@nemaara

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

Fixed naga attack icons.

@nemaara nemaara force-pushed the dunefolk_rework_merge branch from f786ce0 to f678faa Aug 1, 2019

@ghype

This comment has been minimized.

Copy link

commented Aug 1, 2019

Just on a side note: considering how much effort was put into the descriptions @nemaara should be credited for the descriptions as well

@nemaara nemaara force-pushed the dunefolk_rework_merge branch from f678faa to 533074c Aug 2, 2019

@stevecotton stevecotton added this to the 1.15.0 milestone Aug 2, 2019

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

It was naphta, not naphtha in default Dunefolk in Firetrooper description.

Just to be clear, "naphtha" is the correct spelling.

The balance of this faction was based on the newly proposed liminal alignment which works as +0/+0/+25/+0/+0/+25 instead of the old -25/-25/+0/-25/-25/+0

That newly-proposed liminal alignment is in fact merged in master, if I recall correctly. Have these units' stats been adjusted already with that in mind?

@Hejnewar

This comment has been minimized.

Copy link

commented Aug 3, 2019

The balance of this faction was based on the newly proposed liminal alignment which works as +0/+0/+25/+0/+0/+25 instead of the old -25/-25/+0/-25/-25/+0

That newly-proposed liminal alignment is in fact merged in master, if I recall correctly. Have these units' stats been adjusted already with that in mind?

Yes, all liminal units since almost begining of this project were created, adjusted and tested with 0/0/+25/0/0/+25 liminal bonus.

@nemaara

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2019

It seems to me like the Windrider does a bit too much damage; it's melee is 9-3 and ranged is 8-5, which turns into 11-3 and 10-5 at dawn/dusk. This is very high for a mounted unit, so I'm thinking one (or both) of the attacks should have the damage lowered. One might be able to say the same for the Swiftrider, but it might be less of a problem (9-2 melee 7-4 ranged, or 11-2 and 9-4 at dawn and dusk).

@ghype

This comment has been minimized.

Copy link

commented Aug 3, 2019

@nemaara it is to notive that that boost lasts for only one turn, while lawful/chaotic units get boosts for two turns. @Hejnewar would be glad to answer this question though, wether its too much or not

@Hejnewar

This comment has been minimized.

Copy link

commented Aug 4, 2019

@ghype

@nemaara it is to notive that that boost lasts for only one turn, while lawful/chaotic units get boosts for two turns. @Hejnewar would be glad to answer this question though, wether its too much or not

I have already talked about this with nemaara before her commits.

@sevu sevu added the Blocker label Aug 4, 2019

@Pentarctagon

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

Also, keep in mind that everything doesn't have to be absolutely perfect - we have the rest of the 1.15 development cycle to tweak unit stats and iron out any other kinks.

Consolidation of new Dunefolk changes for EarthCake
Wmlindent pass on updated Dunefolk units

[ci skip]

Removed attribution from Dunefolk Alchemist description

[ci skip]

More wmlindent

[ci skip]

Dunefolk Alchemist: revise description

Dune Apothecary: add description

Dune Herbalist: revise description

Dune Luminary: add description

Dune Soldier: add description

Dune Swordsman: revise description

Dune Blademaster: add description

Update the pathfinding tests' expected answers

Dune Spearguard: revise description

Dune Spearmaster: add description

Dune Captain: revise description

Dune Warmaster: add description

Dune Burner: add description

Dune Scorcher: revise description

Dune Firetrooper: add description

Dune Rover: add description

Dune Explorer: add description

Dune Ranger: add description

Dune Skirmisher: revise description

Dune Strider: add description

Dune Harrier: edit description

Dune Paragon: add description

Nagas: change attack icons and descriptions

Naga Bladewhirler: add description

Naga Slasher: add description

Dune Blademaster: increase scimitar damage

Naga Dervish: add description

Naga Ringcaster: add description

Naga Zephyr: add description

Dune Rider: edit description

Dune Cataphract: add description

Dune Sunderer: add description

Dune Swiftrider: add description

Dune Windrider: add description

Dune Marauder: fix attack icons and add description

Dune Raider: revise description and fix attack sounds

Dune Swiftrider: nerf melee damage and exp

Dune Windrider: nerf melee damage

Dunefolk: fix description typos

@Vultraz Vultraz force-pushed the dunefolk_rework_merge branch from 806dfa9 to 4cee28f Aug 9, 2019

@Pentarctagon

This comment has been minimized.

Copy link
Member

commented Aug 10, 2019

Seeing as 1.15.0 is scheduled for tomorrow, can this be merged?

@Vultraz Vultraz merged commit fc961aa into master Aug 10, 2019

1 check failed

continuous-integration/appveyor/pr AppVeyor build failed
Details

@Vultraz Vultraz deleted the dunefolk_rework_merge branch Aug 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.