chiyadev / genshin-schedule

🕑 Genshin farming scheduler
https://genshin.chiya.dev
MIT License
72 stars 17 forks source link

Feature Request: Add Traveler to the character list #11

Open Lambda95 opened 4 years ago

Lambda95 commented 4 years ago

The MC should be part of the characters, possibly as a different character for each element, since talents are leveled independently between variants and with different materialas.

luaneko commented 4 years ago

According to the wikis, it seems like both types of travelers require the same materials (freedom and samachurl scroll)?

Anemo and Geo

I can't verify this myself since I haven't leveled traveler at all.

Lambda95 commented 4 years ago

The wiki is wrong. Anemo uses Resistance for the basic attack and for E, but Ballad for Q. Scrolls for all. Anemo attack talent Anemo E talent Anemo Q talent

Geo meanwhile uses Resistance and scrolls for basic attacks, but Diligence and Arrows for E and Q. Geo attack talent Geo E talent Geo Q talent

Yes, they are all over the place.

luaneko commented 4 years ago

Thanks for verifying! This might require a slight restructure of the database that currently assumes one type of talent material for each character 😓

luaneko commented 4 years ago

Traveler has been added!

Lambda95 commented 4 years ago

The traveler, seems to be even more of a pain in the ass than what I initially thought. Not only does it use different materials for different talents, but it also changes them around based on level... Screenshot (9) Leveling up Geo E to 4 changed from Dilligence to Gold. This might require adding a Talent level to the MC variants, and another restructuring of the database to account for different levels needing different stuff. Sorry for dragging you into this.

Edit: Geo Q also uses Gold for level 4. Screenshot (10)5

Lambda95 commented 4 years ago

After searching around, it seems the Traveler uses every talent book. For Anemo it goes Freedom->Resistance->Ballad for every talent, while Geo uses that cycle for the basic attack talent, and Prosperity->Dilligence->Gold for E and Q.

https://genshin-impact.fandom.com/wiki/Foreign_Ironwind https://genshin-impact.fandom.com/wiki/Foreign_Rockblade

RaffaeleAmmirati commented 4 years ago

If I can help I found this image on reddit for the traveler talent 6 Traveler Talent Info

I have the talent for the new character too.

8 Character Data-Sheets (26)

thank you for your amazing work.

luaneko commented 4 years ago

Traveler has been removed from the character listing for now. I can't think of an elegant way to implement talent materials based on talent level without completely restructuring the database.

Simply assuming all talent materials could also work, but IMO it would be rather confusing to show the domain for a talent material when it's not needed.

RaffaeleAmmirati commented 3 years ago

Hi I have an idea on how it can be possible to add the travel. But I'm not sure if it's impossible to do in a database since I'm not very knowledgeable on programming and database stuff. If we add a checkbox for the talent and a scroll menu for the level it may be possible to add the current anemo and geo traveler and the future traveler too, since the book go in a circle it can be associated with the level that one put in the scroll menu. As you can see in my previous image the problem is not only that the talent book go in a circle but for the geo traveler it change for the melee to the elemental. But with this idea it may be doable. I made a very horrible image with paint XD I hope it's undestandable.

Traveler idea

sorry for my bad english, it's not my native language.

lauslim12 commented 3 years ago

I actually have an idea on how to solve this.

image

In my opinion, rather than thinking about talent levels, what about if we just let the user decide on what items that he/she need? The user can simply click a checkbox that contains the material, then it will be displayed on the main page.

Technically, this is the same like #37, as we have to add the contexts which will store the values of the checkboxes.

What do you think?

luaneko commented 3 years ago

It's a neat idea but I think I would still prefer a talent level input because it makes tracking which material is required easier imo.

The implementation I have in mind right now is a little similar to what you posted, but instead of showing each talent material, it starts off with 3 rows of "None" states. The checkboxes are replaced with numeric inputs at start at 0 (to indicate that no talent materials from domains are required.)

When a talent is at level 0 it is not scheduled. When the user changes it to other levels e.g. 4, the "None" state is updated to "Ballad" and it is automatically scheduled.

lauslim12 commented 3 years ago

It's a good idea!

However, if we do it like that, wouldn't that require a slight refactor of the contexts and the [name] page? Another thing that I noticed is that how do we display them on the main page? Is it by merging the arrays?

luaneko commented 3 years ago

wouldn't that require a slight refactor of the contexts and the [name] page?

The database needs some refactoring to support this, specifically talentMaterials. Talent level info could just be stored as a new separate config characterTalents. This is similar to how notes work.

how do we display them on the main page?

We just need to change the scheduling code in the DomainView component to support talent materials that differ by levels.

lauslim12 commented 3 years ago

I see. Currently, I have no idea of refactoring the talentMaterials besides of changing the talentMaterials to an Object though (without changing the already existing types):

Old one:

export type Character = {
  type: "Character";
  name: string;
  wiki: string;
  talentMaterialWeekly: TalentMaterial[];
  talentMaterials: TalentMaterial[];
  commonMaterials: CommonMaterial[];
};

New one:

type TalentMaterialAndLevels = {
  level: number;
  talent: TalentMaterial;
};

export type Character = {
  type: "Character";
  name: string;
  wiki: string;
  talentMaterialWeekly: TalentMaterial[];
  talentMaterials: TalentMaterialAndLevels[];
  commonMaterials: CommonMaterial[];
};

It's also possible to straight up modify the TalentMaterial though, but we have to refactor the talentMaterialWeekly again if that happens.

The drawback of this approach is that we're going to have to list every levels and its corresponding material, for each character.

luaneko commented 3 years ago

How about like this?

export type Character = {
  type: "Character";
  name: string;
  wiki: string;
  talentMaterialWeekly: TalentMaterial[];
  talentMaterials: (TalentMaterial | TalentMaterialCondition)[]; // discriminated union
  commonMaterials: CommonMaterial[];
};

export type TalentMaterialCondition = {
  type: "Talent Level-Up Material Condition"; // discriminator
  levels: number[];
  material: TalentMaterial;
};

We can keep most the existing code working with this approach.

lauslim12 commented 3 years ago

That's a really good idea!

We can go with your approach!