Skip to content

Update tree data and asset handeling #8295

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

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from

Conversation

Regisle
Copy link
Member

@Regisle Regisle commented Sep 10, 2024

"minor" rework to how we handle and store tree data and assets

The 4 main goals with this are to

  1. make the file structure less cluttered (this isnt entirely achieved with the folder src\TreeData\assets\non_sprite still having alot of loose files, this can be cleaned up more if desired)
  2. dedupe alot of files, this takes the file size for treeData from 258 MB to 142 MB, this is likely something that would continue being a bigger and bigger problem and this helps with that
  3. improve caching and loading of trees, this does slow down the initial load by a tiny amount, but gives over 50x as big a speedup to loading any other trees which arnt the latest tree (mostly helpful for things like ruthless or looking at old builds), this also makes it easier to reduce loading on headless by separating alot of the asset / spritesheet logic out, but likely requires them to fix issues
    There are many other changes that could improve this more, eg not having to redo spritesheets which are duplicates across multiple tree versions, but I have decided to limit the scope of this a bit to make it easier to merge
  4. make it easier to add new trees, this adds logic for moving and renaming of assets, making it easier to just dump the full zip file from GGG every league and more easily have it figure out correct asset files, this could still use some improvements

I have a followup PR which moves most of the logic over to use spritesheets (that were introduced in 3.19), but again I have decided to limit the scope of this a bit to make it easier to merge, and will open that PR once this is merged

@Paliak Paliak added enhancement New feature, calculation, or mod technical Hidden from release notes labels Sep 15, 2024
Copy link
Member

@Wires77 Wires77 left a comment

Choose a reason for hiding this comment

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

This looks mostly fine to me, I just had some nitpicks. I'm almost positive it doesn't work with the alternate trees though :(

Copy link
Member

Choose a reason for hiding this comment

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

Why does the 3.19 Assets become the generic one, but we also have a 3_19.lua file now?

Copy link
Member

Choose a reason for hiding this comment

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

Can we name this folder "sprites" instead of "data"? I had to look at where they were moved from to make sense of it.

-- Retrieve the file at the given Path
local function getFile(Path, altPath)
local text
local File = io.open("TreeData/"..Path..".lua", "r")
Copy link
Member

Choose a reason for hiding this comment

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

nit: Can we keep variable names lowercased if they are just an instance of a class?

Suggested change
local File = io.open("TreeData/"..Path..".lua", "r")
local file = io.open("TreeData/"..Path..".lua", "r")

-- Migrate to old format
class.classes = class.ascendancies
end
class.classes = class.classes or class.ascendancies
Copy link
Member

Choose a reason for hiding this comment

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

Maybe leave a comment in there explaining why this logic is here

Comment on lines +441 to +444
self.legion = main.tree.legion
if not self.legion then
self.legion = LoadModule("Data/TimelessJewelData/LegionPassives")

Copy link
Member

Choose a reason for hiding this comment

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

Is this logic just for being able to skip these loops if the legion nodes have already been loaded?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature, calculation, or mod technical Hidden from release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants