-
Notifications
You must be signed in to change notification settings - Fork 862
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
Fix LDtk tile map resources export with the fast loading #5951
Conversation
* Don't show in changelog
instruction.SetParameter(parameterIndex, updatedParameterValue); | ||
} else if (parameterMetadata.GetType() == "tilemapResource") { | ||
gd::String updatedParameterValue = parameterValue; | ||
worker.ExposeTilemap(updatedParameterValue); | ||
worker.ExposeEmbeddeds(updatedParameterValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why only those 2 types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only these kind of resources are depending on external files.
// TODO Refactor this to avoid to list all project resources as files. | ||
gd::ResourcesManager* resourcesManager = &(project.GetResourcesManager()); | ||
resourceWorker.ExposeResources(resourcesManager); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it's worth adding and doing a
// TODO Refactor this to avoid to list all project resources as files. | |
gd::ResourcesManager* resourcesManager = &(project.GetResourcesManager()); | |
resourceWorker.ExposeResources(resourcesManager); | |
// TODO Refactor this to avoid to list all project resources as files. | |
gd::ResourcesManager* resourcesManager = &(project.GetResourcesManager()); | |
resourceWorker.SetResourcesManager(resourcesManager); |
To avoid all the calls to ExposeResource that will uselessly list all project resources as files?
gd::ResourcesAbsolutePathChecker absolutePathChecker(originalProject.GetResourcesManager(), fs); | ||
gd::ResourceExposer::ExposeWholeProjectResources(originalProject, absolutePathChecker); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is useless since quite a bit of time (it was used years ago to ask the user what to do with absolute filenames, but pretty sure it's not used anymore)
gd::Project& project = originalProject; | ||
if (!updateOriginalProject) { | ||
std::shared_ptr<gd::Project> clonedProject(new gd::Project(originalProject)); | ||
project = *clonedProject; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems incorrect and is probably linked to issue you may have encountered:
A reference can't be changed after it's set. So when you do gd::Project& project = originalProject
, any further assignement will change the originalProject. I.e: when you do project = *cloneProject
, you modify originalProject to be equal to clonedProject
(which is itself made from originalProject).
When a reference is first assigned, it says to which object it points to. But then when you change it, it modifies the object itself, not the reference.
if (updateOriginalProject) { | ||
gd::ResourceExposer::ExposeWholeProjectResources(originalProject, resourcesMergingHelper); | ||
} else { | ||
std::shared_ptr<gd::Project> project(new gd::Project(originalProject)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that I think this is a overly complicated way of writing:
gd::Project project = originalProject; // i.e.: you make a copy.
preserveAbsoluteFilenames, preserveDirectoryStructure); | ||
} else { | ||
gd::Project clonedProject = originalProject; | ||
gd::ProjectResourcesCopier::CopyAllResourcesTo( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicely done!
Demo