Skip to content

Conversation

@haaln
Copy link

@haaln haaln commented Oct 30, 2025

The original README example contained a small syntax/usage error that could confuse users attempting to compile the sample code directly.

This update improves the reliability of the documentation.

haaln added 2 commits October 30, 2025 12:14
Fixed minor syntax issue in C++ example code to improve clarity.
@syoyo syoyo requested a review from Copilot October 30, 2025 18:19
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the README.md documentation to improve the example code snippet for using TinyGLTF. The example code is wrapped in a complete main() function to make it more practical for users to compile and run directly.

Key changes:

  • Wrapped the example code in a proper main() function with command-line argument handling
  • Moved the using namespace tinygltf; declaration inside the function scope
  • Added explicit return 0; for successful execution

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

README.md Outdated
#include "tiny_gltf.h"

using namespace tinygltf;
int main(int /* argc */, char *argv[]){
Copy link

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

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

[nitpick] The function definition should have a space before the opening brace. Change main(int /* argc */, char *argv[]){ to main(int /* argc */, char *argv[]) { to follow standard C++ formatting conventions.

Suggested change
int main(int /* argc */, char *argv[]){
int main(int /* argc */, char *argv[]) {

Copilot uses AI. Check for mistakes.
README.md Outdated
#include "tiny_gltf.h"

using namespace tinygltf;
int main(int /* argc */, char *argv[]){
Copy link

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

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

The argc parameter is commented out but the code uses argv[1] on line 164 without checking if it exists. This will cause a segmentation fault if the program is run without arguments. Either uncomment argc and add a validation check like if (argc < 2), or document that the program requires at least one argument.

Suggested change
int main(int /* argc */, char *argv[]){
int main(int argc, char *argv[]){
if (argc < 2) {
printf("Usage: %s <input.gltf>\n", argv[0]);
return -1;
}

Copilot uses AI. Check for mistakes.
@haaln
Copy link
Author

haaln commented Oct 31, 2025

Reviewing the code example in the README, I tried to infer its intent behind it while keeping it as concise as possible, since there are plenty of examples available.

As Copilot pointed out, the example will segfault unless arguments are provided. However, I felt adding this detail would clutter the example unnecessarily.

@syoyo, what are your thought's on this?

@syoyo
Copy link
Owner

syoyo commented Oct 31, 2025

To make code minimal, I think simply use filename instead of argv[1] should suffice to show the use of TinyGLTF API.
(and remove return -1 in the last part)

so the code snippet will be like this:

// Define these only in *one* .cc file.
#define TINYGLTF_IMPLEMENTATION
#define STB_IMAGE_IMPLEMENTATION
#define STB_IMAGE_WRITE_IMPLEMENTATION
// #define TINYGLTF_NOEXCEPTION // optional. disable exception handling.
#include "tiny_gltf.h"

using namespace tinygltf;

Model model;
TinyGLTF loader;
std::string err;
std::string warn;

std::string filename = "input.gltf"; // Your glTF filename

bool ret = loader.LoadASCIIFromFile(&model, &err, &warn, filename);
//bool ret = loader.LoadBinaryFromFile(&model, &err, &warn, filename); // for binary glTF(.glb)

if (!warn.empty()) {
  printf("Warn: %s\n", warn.c_str());
}

if (!err.empty()) {
  printf("Err: %s\n", err.c_str());
}

if (!ret) {
  printf("Failed to load glTF: %s\n", filename.c_str());
}

@haaln
Copy link
Author

haaln commented Oct 31, 2025

Alright, I see what you mean. It seems this PR would mostly bring us back to the previous implementation, so it’s probably redundant.

Thanks for your response and work on the library.

Closing this PR.

@haaln haaln closed this Oct 31, 2025
syoyo added a commit that referenced this pull request Nov 2, 2025
@syoyo
Copy link
Owner

syoyo commented Nov 2, 2025

Modified the usage in this commit: 6d8bba0

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