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

Reworking copying functions to only copy files for textures #264

Merged
merged 12 commits into from
Apr 20, 2024

Conversation

klingbolt
Copy link
Contributor

@klingbolt klingbolt commented Jan 5, 2024

The goal of this PR was to make it explicit when we want to copy images from an input folder to an output folder. Since this is adding copying files from the new protobin folders, I standardized how those are read to match the taco directories.

@AsherGlick
Copy link
Owner

When would we have a situation where we have an output file and we dont want to copy files?

@klingbolt
Copy link
Contributor Author

We had previously mentioned if you are using the program simply as a linter, you would not want to copy them over.

Perhaps we want to make that the optional case then and default to copying them.

@AsherGlick
Copy link
Owner

And in that conversation we had also come to the conclusion that if you were using the xml_converter as only a linter you would not have an output file.

@klingbolt
Copy link
Contributor Author

Ah I had not remembered that. I'll make adjustments.

xml_converter/src/xml_converter.cpp Outdated Show resolved Hide resolved
xml_converter/src/xml_converter.cpp Outdated Show resolved Hide resolved
xml_converter/src/xml_converter.cpp Outdated Show resolved Hide resolved
xml_converter/src/xml_converter.cpp Outdated Show resolved Hide resolved
@klingbolt klingbolt changed the title Added argument for copying files and standardized proto reading Reworking copying functions to only copy files for textures Jan 12, 2024
@klingbolt
Copy link
Contributor Author

This change should now copy only the files that used as textures. In addition, the ability to read proto texture indexes was added.

@klingbolt klingbolt requested a review from AsherGlick January 12, 2024 04:11
xml_converter/src/attribute/image.cpp Outdated Show resolved Hide resolved
xml_converter/src/attribute/image.cpp Outdated Show resolved Hide resolved
xml_converter/src/attribute/image.cpp Outdated Show resolved Hide resolved
xml_converter/src/packaging_protobin.cpp Outdated Show resolved Hide resolved
xml_converter/src/packaging_protobin.cpp Outdated Show resolved Hide resolved
xml_converter/src/xml_converter.cpp Outdated Show resolved Hide resolved
xml_converter/src/xml_converter.cpp Outdated Show resolved Hide resolved
xml_converter/src/xml_converter.cpp Outdated Show resolved Hide resolved
xml_converter/src/xml_converter.cpp Outdated Show resolved Hide resolved
xml_converter/src/xml_converter.cpp Outdated Show resolved Hide resolved
Copy link
Owner

@AsherGlick AsherGlick left a comment

Choose a reason for hiding this comment

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

I reviewed part of this but it looks like there are some changes that are not totally necessary or thought out. I'm going to pass it back to you with only whatever notes I have so far so you can go over it again yourself first before sending it back again.

For example you changed a state variable from a string to a cstr and then because of that change you added a bunch of extra random function calls to deal with everything that cstr interacted with expecting a string.

Another is that you took the name filedir from the proto writer state struct and replaced other more descriptive names. I am not really sure what filedir is and probably that variable name should have been changed instead of being propagated throughout the codebase.

xml_converter/src/packaging_xml.cpp Outdated Show resolved Hide resolved
xml_converter/src/packaging_protobin.hpp Outdated Show resolved Hide resolved
xml_converter/src/packaging_protobin.hpp Outdated Show resolved Hide resolved
xml_converter/src/packaging_protobin.cpp Outdated Show resolved Hide resolved
xml_converter/src/state_structs/xml_reader_state.hpp Outdated Show resolved Hide resolved
xml_converter/src/packaging_xml.cpp Outdated Show resolved Hide resolved
xml_converter/src/state_structs/xml_writer_state.hpp Outdated Show resolved Hide resolved
xml_converter/src/attribute/trail_data.cpp Outdated Show resolved Hide resolved
xml_converter/src/packaging_protobin.cpp Outdated Show resolved Hide resolved
xml_converter/src/packaging_xml.cpp Outdated Show resolved Hide resolved
xml_converter/src/state_structs/xml_reader_state.hpp Outdated Show resolved Hide resolved
xml_converter/src/state_structs/xml_writer_state.hpp Outdated Show resolved Hide resolved
xml_converter/src/packaging_protobin.cpp Outdated Show resolved Hide resolved
xml_converter/src/packaging_protobin.cpp Outdated Show resolved Hide resolved
xml_converter/src/packaging_xml.cpp Outdated Show resolved Hide resolved
xml_converter/src/xml_converter.cpp Outdated Show resolved Hide resolved

Choose a reason for hiding this comment

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

Full Diff
--- xml_converter/integration_tests/test_cases/texture/output_proto/markers.bin.textproto._old	2024-03-18 01:20:59.122077476 +0000
+++ xml_converter/integration_tests/test_cases/texture/output_proto/markers.bin.textproto._new	2024-03-18 01:20:59.130077442 +0000
@@ -21,11 +21,11 @@
 textures {
 }
 textures {
-  filepath: "my_texture.png"
+  filepath: "texture_one.png"
 }
 textures {
-  filepath: "my_texture2.png"
+  filepath: "texture_two.png"
 }
 textures {
-  filepath: "somedir/my_texture3.png"
+  filepath: "somedir/texture_three.png"
 }

@klingbolt klingbolt requested a review from AsherGlick April 2, 2024 22:44

Choose a reason for hiding this comment

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

Full Diff
--- xml_converter/integration_tests/test_cases/texture/output_proto/markers.bin.textproto._old	2024-04-20 22:56:53.572116768 +0000
+++ xml_converter/integration_tests/test_cases/texture/output_proto/markers.bin.textproto._new	2024-04-20 22:56:53.580116633 +0000
@@ -21,11 +21,11 @@
 textures {
 }
 textures {
-  filepath: "my_texture.png"
+  filepath: "texture_one.png"
 }
 textures {
-  filepath: "my_texture2.png"
+  filepath: "texture_two.png"
 }
 textures {
-  filepath: "somedir/my_texture3.png"
+  filepath: "somedir/texture_three.png"
 }

@AsherGlick AsherGlick merged commit 595ffdf into AsherGlick:xml_converter Apr 20, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants