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

Show races as nestable subfolders in help #7228

Merged
merged 1 commit into from Jan 18, 2023

Conversation

stevecotton
Copy link
Contributor

@stevecotton stevecotton commented Dec 24, 2022

Edit: no longer just a prototype.

Prototype: This was the original idea for visualising [race]help_taxonomy=, it didn't feel right to me back in September 2020, but I can't remember why it didn't. That idea is now being discussed in https://r.wesnoth.org/t56522 , so I'm opening a PR to share the implementation.

@stevecotton stevecotton added UI User interface issues, including both back-end and front-end issues. Units Issues that involve unit definitions or their implementation in the engine. Help In-game Help functions labels Dec 24, 2022
@stevecotton stevecotton self-assigned this Dec 24, 2022
@CelticMinstrel
Copy link
Member

A screenshot would be nice.

@stevecotton
Copy link
Contributor Author

There's not really much to see in the screenshot, this is just an engine change so the only races affected are Dunefolk and Quenoth. Beetlenaut's ASCII art in https://r.wesnoth.org/p678343 is probably a better illustration.
image

@doofus-01
Copy link
Member

We can (and will) bicker over the organization, but this looks like it should take care of the engine capability.

@CelticMinstrel
Copy link
Member

Agreed. It looks reasonable enough. I guess the only question is whether additional levels of nesting are required, but from the forum thread it doesn't seem like they are needed at this point.

src/help/help_impl.cpp Outdated Show resolved Hide resolved
@stevecotton
Copy link
Contributor Author

If a race has units with variations, and also has subgroups then the section-headings get mixed together. I'm thinking this is something to accept for now, on the assumption that the units with variations will be moved to subgroups before 1.18.

I changed Bats to be a subgroup of Undead for testing:
image

@doofus-01 doofus-01 mentioned this pull request Dec 30, 2022
@doofus-01
Copy link
Member

This brings up a question, what should we do with zombie bats? (Or any cross-racial variation?)
Vampire bats are just a type of bat, but WC/Soulless bats are both bats and zombies. Maybe we should use this as a chance to consolidate inheritance pathways - > deprecate [variation] and just use [base_unit] where needed?

For example, UtBS' Quenoth Elves now appear as a subfolder of Elves, and a major
reorganisation of animals is being discussed in https://r.wesnoth.org/t56522 .

Multiple levels of nesting are supported. If [race]help_taxonomy= points to a
race that isn't defined, or isn't defined with the current set of add-ons, then
it will fall back to adding the race at the top level. Discovering a unit
belonging to a subgroup will automatically treat the parent group as discovered
too.

I may have missed a trick in writing this nicely, the code uses several different
traversal patterns over the tree that it's constructing.
@stevecotton
Copy link
Contributor Author

Adding a [unit_type]help_taxonomy= to add individual units to multiple races is possible, not sure if it's a good idea.

@knyghtmare
Copy link
Member

I do not think [variation] should be deprecated. The ramifications of such a change is...questionably significant...two mainline campaigns rely on [variation] for adding in AMLA unit trees. One mainline (HttT) campaign uses it for incorporating a key plot element. This is just, mainline. UMC...? this will probably break quite a few, notably the popular ones such as Genesis or the Dragon Trilogy ones. Perhaps many more.

but WC/Soulless bats are both bats and zombies.

Once victim to the plague, the resultant being is an undead.

Adding a [unit_type]help_taxonomy= to add individual units to multiple races is possible

Not really seeing the usage scope of apart from the WC/Soulless, maybe some the horse-mount units.

@stevecotton stevecotton changed the title Prototype showing races as nestable subfolders in help Show races as nestable subfolders in help Dec 30, 2022
@stevecotton stevecotton marked this pull request as ready for review December 30, 2022 05:23
@doofus-01
Copy link
Member

I do not think [variation] should be deprecated. The ramifications of such a change is...questionably significant...two mainline campaigns rely on [variation] for adding in AMLA unit trees. One mainline (HttT) campaign uses it for incorporating a key plot element. This is just, mainline. UMC...? this will probably break quite a few, notably the popular ones such as Genesis or the Dragon Trilogy ones. Perhaps many more.

Some things may use [variation], but nothing relies on it. Same as anything else that has been deprecated.

Adding a [unit_type]help_taxonomy= to add individual units to multiple races is possible, not sure if it's a good idea.

That flexibility is good to have.

@ForestDragon-wesnoth
Copy link
Contributor

As a UMC creator myself, I completely agree with knyght here. Such changes just further inconveniences UMC creators, and the more changes are required to port add-ons to new versions the less people will bother porting add-ons. Between new versions, the number of add-ons is slowly shrinking. If you can't attract new creators the least you can do is not unnecessarily hinder the work of the few people still sticking around.

@MechanicalDragon963
Copy link
Contributor

This brings up a question, what should we do with zombie bats? (Or any cross-racial variation?) Vampire bats are just a type of bat, but WC/Soulless bats are both bats and zombies. Maybe we should use this as a chance to consolidate inheritance pathways - > deprecate [variation] and just use [base_unit] where needed?

Don't touch the [variation] tag. No need to destroy a lot of addons for the sake of a cosmetic change to the wiki. It's not worth it.

I'm sure there are other solutions to this problem. It's worth thinking about them.

@NorcensTomatoes
Copy link

Created a Github Account to say as a new UMC author (campaign is in the works) over 70% of my code relies upon [variation], I've been working on my campaign for the past several months juggling between it, school, and work. Due to how much work I've put into it I would find it very heartbreaking to see the whole thing no longer working because of a cosmetic change that hardly benefits anyone, and I know several other authors in similar boats to me. I don't think it's appropriate to remove a key like [variation] which is used so heavily as it will greatly impact the work of newer devs along with older devs and mainline content.

@CelticMinstrel
Copy link
Member

CelticMinstrel commented Dec 31, 2022

I'm not sure if anyone actually suggested it I only just noticed the sneaky mention suggesting it, but I don't think there's any need to deprecate the [variation] tag. Using it for the Sand Scuttler specifically was in retrospect seen as not the best idea, but I don't know of any argument that applies to the tag in general.

At most we may want to write a guideline on its use, something like:

In general, the variation tag should not be used to arbitrarily classify one unit under another in the help pages. It should be used primarily only for variations that are chiefly visual in nature, with no mechanical changes.

As a partial exception to the preceding guideline, it is also expected and normal to use variations for a unit that is created by a plague weapon special, even if the variations have potentially significant mechanical differences.

@doofus-01
Copy link
Member

For the record, the suggestion to remove [variation] wasn't "sneaky", and as a UMC author myself I know deprecations are annoying, but you all are making this sound like a much bigger proposed change than it is, and the wiki has nothing to do with anything. In any case, it's not a part of this PR, so it wouldn't happen here.

@stevecotton
Copy link
Contributor Author

There's a lot of discussion about future directions, but IIUC the status of the PR itself is that there's no objection to what this PR is doing. Please could I have a few thumbs-up votes to confirm that?

There's still a question for @Vultraz about whether the code itself looks good.

If a race has units with variations, and also has subgroups then the section-headings get mixed together. I'm thinking this is something to accept for now, on the assumption that the units with variations will be moved to subgroups before 1.18.

Clarifying what I meant by that, now that I've seen it can be read it two ways: I meant it looked wrong when the list went

  • Undead
    • Soulless
      • (variations)
    • Walking corpse
      • (variations)
    • Physical undead
      • Skeleton
      • etc
    • Spectral undead
      • Ghost
      • etc

But that would sort itself out when the WC and Soulless were moved into the Physical Undead folder by adding a [unit_type]help_taxonomy= to the base WC and Soulless (with no change needed for any of their variations). So then it would be:

  • Undead
    • Physical undead
      • Soulless
        • (variations)
      • Walking corpse
        • (variations)
      • Skeleton
      • etc
    • Spectral undead
      • Ghost
      • etc

Copy link
Member

@Vultraz Vultraz left a comment

Choose a reason for hiding this comment

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

No complaints code-wise. I defer to the whatever consensus there is as to tree layout.

@stevecotton
Copy link
Contributor Author

I now think that adding [unit_type]help_taxonomy= is a necessary extra if we want to apply this to the mainline humans and undead. However, that should be for a separate PR.

The current PR makes [race]s cosmetically nestable, allowing "Monsters/Scorpions" for [unit_type]race=scorpion etc. However it doesn't allow subdivisions of a race; so everything with [unit_type]race=undead ends up in the same folder. We're definitely not changing the race=undead line, so a new [unit_type]help_taxonomy= would be needed if we want to split the physical ones from the ethereal ones.

Walking Corpses would get [unit_type]help_taxonomy=undead_corpse, along with the Ghoul tree. UMC units such as AE's Animated Corpse (AE_mag_Walking_Corpse) would still be listed in the "Undead" instead of "Undead/Corpses" unless that [unit_type]help_taxonomy=undead_corpse is added to them too. With no changes to AE, but after adding a follow-up PR with [unit_type]help_taxonomy=, the help section would look like the image below. Imagine there are "Necromancers", "Skeletal" and "Spectral" folders too.
image

The only side-effect on [variation] is that the base unit's taxonomy determines where in the documentation tree that base unit appears. The variations still appear as a sub-folder under main unit.

@knyghtmare, @cooljeanius, @ForestDragon-wesnoth, @MechanicalDragon963 and @NorcensTomatoes - with this and the clarification in my previous post, do you have any objections to the current PR, or to that follow-up?

@cooljeanius
Copy link
Contributor

@knyghtmare, @cooljeanius, @ForestDragon-wesnoth, @MechanicalDragon963 and @NorcensTomatoes - with this and the clarification in my previous post, do you have any objections to the current PR, or to that follow-up?

I'm not quite sure... I just want to avoid stuff getting broken, and am not sure if "side-effect" can be parsed as "breakage" or not...

@knyghtmare
Copy link
Member

with this and the clarification in my previous post, do you have any objections to the current PR, or to that follow-up?

as long as the current system with [variation] is unchanged, it's agreeable.

This PR is just a start, I assume since the monsters race can be split into a multitude of other races (ants/scorpions/birds/elementals/cephalods) once time permits?

@stevecotton
Copy link
Contributor Author

This PR is just the help browser UI, it will work for reorganising the monsters too.

Some of the images show the results of adding [unit_type]help_taxonomy=, but those additions aren't actually included in this PR.

@stevecotton stevecotton merged commit a4e17ac into wesnoth:master Jan 18, 2023
@stevecotton stevecotton deleted the unit_help_taxonomy_tree branch January 18, 2023 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Help In-game Help functions UI User interface issues, including both back-end and front-end issues. Units Issues that involve unit definitions or their implementation in the engine.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants