Importer and Edito UI Changes#779
Conversation
…a to be used for affix_tab limitation by slot
cjshrader
left a comment
cjshrader
left a comment
There was a problem hiding this comment.
This is as far as I could get this morning. I still need to review the affixes_tab onward (which is a lot, to be honest). I have a few more non-specific comments as well that I'll add after this
| class AffixFilterModel(AffixAspectFilterModel): | ||
| model_config = ConfigDict(extra="forbid", populate_by_name=True) | ||
| want_greater: bool = False | ||
| required: bool = False |
There was a problem hiding this comment.
I don't know what this required flag is but it doesn't seem relevant to a profile editor and importer overhaul. I'm unclear how this works differently than adding another count block, which you also support.
There was a problem hiding this comment.
The required option would let you set a specific affix to required which would be along with any count settings you might have.
There was a problem hiding this comment.
The count block already accomplishes this, it's not good to add new functionality here where it can be avoided
| min_greater_affix_count: int = Field(default=0, alias="minGreaterAffixCount") | ||
| min_percent_of_aspect: int = Field(default=0, alias="minPercentOfAspect") | ||
| min_power: int = Field(default=0, alias="minPower") | ||
| item_type: list[ItemType] = Field(default=[], alias="itemType") |
There was a problem hiding this comment.
These make no sense here and were already removed previously and intentionally
There was a problem hiding this comment.
the code uses this field to immediately skip items that don't match the intended item category.
There was a problem hiding this comment.
Why would a user want to say "Keep all unique helmets". It literally won't happen, which is why it was removed previously.
And the other functionality is similarly unnecessary or was added to the Affixes.
| if len({aspect.name for aspect in self.unique_aspect}) != len(self.unique_aspect): | ||
| msg = "uniqueAspect names must be unique" | ||
| raise ValueError(msg) | ||
| if not self.affix_pool: |
There was a problem hiding this comment.
affix_pool can never be none, what is this trying to accomplish?
There was a problem hiding this comment.
This line is a safeguard that ensures the filtering engine has a structured object to work with, even when the user provides minimal configuration. It turns "no affix requirements" into an explicit "zero-requirement match" so the program logic remains consistent.
There was a problem hiding this comment.
I'm sorry I don't know what that's saying. affix_pool has a default value of []. There is no situation where it will need to be reinitialized. If there is a situation we probably need to fix a problem elsewhere
| ) | ||
| tributes: list[TributeFilterModel] = Field(default=[], alias="Tributes") | ||
| paragon: dict[str, object] | list[dict[str, object]] | None = Field(default=None, alias="Paragon") | ||
| source_url: str = Field(default="", alias="SourceUrl") |
There was a problem hiding this comment.
Can you make sure anywhere the paragon board used a source_url is either removed or it uses this one? I think it only stored it for no particular reason and can probably just get removed
|
|
||
| if slot_to_unique_name_map[slot]: | ||
| unique_name, rarity = slot_to_unique_name_map[slot] | ||
| if rarity == ItemRarity.Mythic: |
There was a problem hiding this comment.
We don't import affixes for mythics. This change alone has us import affixes. You can look at how the code was before I implemented multiple aspects if you want to see how to handle this
There was a problem hiding this comment.
And note this applies to all of the importers
| worker.signals.finished.connect(finish_callback) | ||
|
|
||
| THREADPOOL.start(worker) | ||
| QMessageBox.information(self, "Refresh Profile", f"Refreshing '{name}' in the background...") |
There was a problem hiding this comment.
What if there are warning or errors on the refresh? How do we surface those to the user?
cjshrader
left a comment
cjshrader
left a comment
There was a problem hiding this comment.
Additional issues:
It appears to me if someone has no item type defined then it all ends up in helm
Example profile info, it's part of Affixes:
Affixes:
- Mythics:
min_power: 900
unique_aspect:
- {name: heir_of_perdition}
- {name: ring_of_starless_skies}
- {name: melted_heart_of_selig}
Also on saving your code added an affix_pool to this when an affix pool is not required.
I know we've talked about this but our favorite little stick guy is taking up a full 50% of the screen 2560x1440 . This is just not user friendly. 90% of the work is done on the right hand side yet it's shoved over
Unique aspects are taking up much more space than they need. Most items won't even have one, and the ones that do have one will rarely have more than one. We definitely need a different solution that doesn't take up 1/2 of the available remaining space.
My profile has a second sword that's not being displayed here. It also has a missing second two hander. And my profile is older so it doesn't know it's barbarian.
Say I want to start searching for flails. How do I do that? I'll attach the entire profile.
maxroll_barbarian_s13_whirlwind_endgame_barbarian_selig_overpower.yaml
You've changed how Convert to Min % works (it only used to work on the specific item you had open) and I don't have strong feelings on that but when I pressed it it did not convert my chest armor, polearm, or sword (sword2 was but not "sword")
|
One more comment is the color scheme of this page is totally different than the color scheme of everything else you've reworked. I'd feel like it should be consistent but open to your thoughts there. |
2 participants
explorer_7X9NRFo5rv.mp4
This includes a class_name and source_url in the build yaml.