Skip to content

Remove print statements from FBX plugin #1777

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

Merged
merged 2 commits into from
Feb 25, 2022
Merged

Remove print statements from FBX plugin #1777

merged 2 commits into from
Feb 25, 2022

Conversation

wyskoj
Copy link
Contributor

@wyskoj wyskoj commented Feb 23, 2022

This changes a print statement in FbxAnimCurveNode.java to a logger call so it doesn't contaminate output.

Change a print statement in FbxAnimCurveNode.java to a logger call
@stephengold
Copy link
Member

Thank you for proposing this change.

The FBX loader contains 13 print statements directed to System.out. Why does this particular print statement cause an issue?

Does the issue relate to FbxDump.dumpFile() or is there some other impact?

@wyskoj
Copy link
Contributor Author

wyskoj commented Feb 24, 2022

I spent some time resolving warnings logged by JME in my project, and noticed I kept getting this print statement. So this was the only one that affected me, but if you prefer, I could do the same to the rest of the occurrences.

Just wanted to change this merely for the fact of cleaning up my console log.

@stephengold
Copy link
Member

The rogue message isn't a warning. I suspect it's someone's old temporary debugging code.

So, just to be sure I understand ... the only impact was unnecessary messages on the console, which might've made actual diagnostics easy to overlook?

@wyskoj
Copy link
Contributor Author

wyskoj commented Feb 24, 2022

I know it's not a warning. In the process of resolving warnings, I spotted the output of this print statement in my console. And it doesn't help me to have it there (and frankly I'd like to have it gone). And by making it a fine call, it can still be seen—if wanted—by setting the logger level.

So, yes, that is the the only impact—just me being picky on the cleanliness of my console :)

@stephengold
Copy link
Member

Thank you for your patience. Now that I understand the issue/motivation, I'm OK with this PR as is.

However, I notice that the straightforward conversion of println() to Logger.fine() results in some inefficient code: converting floats to strings, concatenating them, and then (in the usual case) not using the result. If you're agreeable, I suggest simply commenting out the original println(), similar to what was done in FbxLoader.constructAnimations().

@wyskoj
Copy link
Contributor Author

wyskoj commented Feb 24, 2022

Yes, I do agree. An alternative would be to define the string lazily. But I don't suspect these statements are getting enough attention to warrant implementing that. I'll comment them out.

@wyskoj wyskoj changed the title Fix rogue print statement in FbxAnimCurveNode Remove print statements from FBX plugin Feb 24, 2022
Copy link
Member

@stephengold stephengold left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for your cooperation and thoroughness!

@stephengold stephengold added this to the Future Release milestone Feb 24, 2022
@stephengold stephengold merged commit 989dbfe into jMonkeyEngine:master Feb 25, 2022
@Ali-RS Ali-RS modified the milestones: Future Release, v3.6.0 Dec 31, 2022
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.

3 participants