-
Notifications
You must be signed in to change notification settings - Fork 196
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
Optimize ImageLoaderSVG::create_image_from_utf8_buffer #137
Optimize ImageLoaderSVG::create_image_from_utf8_buffer #137
Conversation
Is this still being worked on? |
If a further udpate is requested from reviewers, I will consider it. Maybe adding a lot of lines for the optimized code in |
Well the most notable thing is you need to squash the commits, and yeah making it more readable would definitely be desirable, also does this have a corresponding Godot PR or proposal? And what is your opinion on having submitting this upstream to Godot? (presuming it hasn't already been tried) |
My understanding is that maintainers who have rights to merge PRs can choose
Then I will create a function to isolate the optimized code and call the newly created function from
I still haven't created a PR on https://github.com/godotengine/godot project.
I'm not really sure if it's absolutely necessary but if maintainers/reviewers want it to happen, I'll open a PR (or an issue) on the original project. Please be aware that merging this PR doesn't benefit users who use Pre-Built xxdot Engine Binaries. (The reason is written on #137 (comment)) |
Githubs documentation is full of problems and contradicts itself regularly, as well what that actually does is eliminates references to the PR and the original commit which its own form of pain, its not just review that's easier, but also it makes the management of history and reverts easier, plus its the only way to be consistent, Github's PR squash behavior is functionally useless and unusable for any real git workflow. We follow Godot's PR workflow which requires you to squash your commits anyway.
Unless you have an opposition to doing so, its usually preferable to push things that haven't been submitted to Godot there first so that we can minimize merge conflicts when we merge back from upstream, we want to focus more on stalled, abandoned, and rejected PRs (and/or proposals) over specifically just being distinct from Godot by having specific distinct contributors.
Well yeah that's not really a big deal, Godot, and thus Redot, has plenty of things that aren't produced in the pre-built binaries anyway, some specific workflows (like encrypted pcks) already require compiling it yourself anyhow. |
Wow, I didn't know GitHub has those issues. They must have been frustrating to some users.
It's totally fine for me to do so. It's just I wasn't aware it's the preferable way. So I'll close this PR and create an issue (and a PR) to the original project. If the PR on the original project wouldn't be merged after several years, I might reopen this PR.
Glad to hear that. Thank you for explaining a lot of things to a new comer. |
There's no need to close this PR, as there is potential for it to still be considered here, we just want to see what happens when submitted to Godot first for things that haven't been there yet, (given the PR author is fine with it going upstream) if it takes too long (we'd definitely consider it well before a few years pass, likely before a few months pass) or gets rejected, we're still likely to consider this, also I'm not entirely sure this would need a Godot proposal, I'm pretty sure you can just submit a PR for it. |
1b5f96b
to
10fb526
Compare
I've just opened a PR on the Godot project. |
10fb526
to
141f75e
Compare
Upstream approved the PR but still waiting on them to merge. We don't have a nailed down timeline we'll wait for Godot to merge stuff like this, as we can't have our contributors waiting 3 months on Godot, but we'll keep an eye on it when we have time and discuss when to let Godot be lame and pull the trigger on our own. |
godotengine/godot#98329 has been merged so I'll close this one. |
This PR optimizes
ImageLoaderSVG::create_image_from_utf8_buffer
method.I built the engine with the following command.
And profiled CPU usage with Microsoft Visual Studio's Perfromance Profiler by launching
redot.windows.editor.x86_64.exe
file. A little bif of performance improvement can be confirmed.(Tested with AMD Ryzen 9 7900X processor)
ImageLoaderSVG::create_image_from_utf8_buffer
CPU usage (%)