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

Add stretchable sprite component and corresponding sample #2214

Closed
wants to merge 5 commits into from

Conversation

@ricab
Copy link
Contributor

commented Dec 22, 2017

No scripting bindings ATM. Help with those would be great. Code review and testing would also be appreciated.

Making StretchableSprite2D inherit from StaticSprite2D was the easiest way to avoid code duplication, but it meant inheriting several "degrees of freedom" (e.g. manual hotspot, draw rect, spritesheet, flips), with many combinations that I could not test.

See related discussion in https://discourse.urho3d.io/t/stretchable-sprites/3842

@ricab

This comment has been minimized.

Copy link
Contributor Author

commented Dec 27, 2017

Scripting bindings done (both AngelScript and Lua)

@ricab

This comment has been minimized.

Copy link
Contributor Author

commented Dec 27, 2017

Hi, I did part of this outside of my normal dev environment and just noticed that my editor introduced a bunch of white space errors. They are now fixed in the last commit.

@weitjong weitjong force-pushed the urho3d:master branch 4 times, most recently from c7346fe to b46c83f Jan 17, 2018

@weitjong weitjong self-assigned this Jan 24, 2018

@weitjong weitjong closed this in d368227 Jan 25, 2018

@weitjong

This comment has been minimized.

Copy link
Member

commented Jan 25, 2018

Thanks for your PR.

urho3d-travis-ci added a commit that referenced this pull request Jan 26, 2018

Travis CI: API documentation update at 2018-01-26 00:40:51 UTC.
[ci package]

Commit: d368227

Message: Minor formatting and modernizing the code.
Close #2214.

urho3d-travis-ci added a commit that referenced this pull request Jan 26, 2018

Travis CI: API documentation update at 2018-01-26 00:40:51 UTC.
[ci package]

Commit: d368227

Message: Minor formatting and modernizing the code.
Close #2214.

urho3d-travis-ci added a commit that referenced this pull request Jan 26, 2018

Travis CI: API documentation update at 2018-01-26 00:40:51 UTC.
[ci package]

Commit: d368227

Message: Minor formatting and modernizing the code.
Close #2214.

urho3d-travis-ci added a commit that referenced this pull request Jan 26, 2018

Travis CI: API documentation update at 2018-01-26 00:40:51 UTC.
[ci package]

Commit: d368227

Message: Minor formatting and modernizing the code.
Close #2214.

urho3d-travis-ci added a commit that referenced this pull request Jan 26, 2018

Travis CI: API documentation update at 2018-01-26 00:40:51 UTC.
[ci package]

Commit: d368227

Message: Minor formatting and modernizing the code.
Close #2214.

urho3d-travis-ci added a commit that referenced this pull request Jan 26, 2018

Travis CI: API documentation update at 2018-01-26 00:40:51 UTC.
[ci package]

Commit: d368227

Message: Minor formatting and modernizing the code.
Close #2214.

urho3d-travis-ci added a commit that referenced this pull request Jan 26, 2018

Travis CI: API documentation update at 2018-01-26 00:40:51 UTC.
[ci package]

Commit: d368227

Message: Minor formatting and modernizing the code.
Close #2214.

urho3d-travis-ci added a commit that referenced this pull request Jan 26, 2018

Travis CI: API documentation update at 2018-01-26 00:40:51 UTC.
[ci package]

Commit: d368227

Message: Minor formatting and modernizing the code.
Close #2214.

urho3d-travis-ci added a commit that referenced this pull request Jan 26, 2018

Travis CI: API documentation update at 2018-01-26 00:40:51 UTC.
[ci package]

Commit: d368227

Message: Minor formatting and modernizing the code.
Close #2214.
@ricab

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2018

Thank you @weitjong! And sorry for the formatting differences. BTW, do you use any formatter tool with a Urho-compatible profile? Cheers

@weitjong

This comment has been minimized.

Copy link
Member

commented Jan 26, 2018

I have customized my IDE (Clion) to match the existing code style some years back and now just invoke the IDE to format for me. Still need to manual adjust it afterwards though. So, it is still not perfect. In future we will employ Clang-Format to check the code style when the PR is being raised.

weitjong added a commit that referenced this pull request Jan 26, 2018

Travis CI: API documentation update at 2018-01-26 00:40:51 UTC.
[ci package]

Commit: d368227

Message: Minor formatting and modernizing the code.
Close #2214.

weitjong added a commit that referenced this pull request Jan 26, 2018

Travis CI: API documentation update at 2018-01-26 00:40:51 UTC.
[ci package]

Commit: d368227

Message: Minor formatting and modernizing the code.
Close #2214.

haolly added a commit to haolly/Urho3D that referenced this pull request Jan 27, 2018

haolly added a commit to haolly/Urho3D that referenced this pull request Jan 27, 2018

Travis CI: API documentation update at 2018-01-26 00:40:51 UTC.
[ci package]

Commit: urho3d@d368227

Message: Minor formatting and modernizing the code.
Close urho3d#2214.
@ricab

This comment has been minimized.

Copy link
Contributor Author

commented Jan 27, 2018

Note: coverity reports 4 new null ptr dereference defects in generated code for lua bindings. I believe they can be safely ignored as they are the same as for other Urho2D components (that particular file has 641 issues). Also, some null ptr checks are done beforehand (unless TOLUA_RELEASE is defined). Let me know if they should be marked ignored, I can do that (assuming I have permissions).

@weitjong

This comment has been minimized.

Copy link
Member

commented Jan 27, 2018

Yes, you can safely ignore them. Probably it makes sense to setup the scan without the tolua++ generated code for now to remove all those defects reports from coverity scan, instead of going though them manually.

@weitjong

This comment has been minimized.

Copy link
Member

commented Jan 28, 2018

The last scan without Lua build option enabled and hence without those tolua++ generated code has surprisingly drastically eliminated 6,160 defects in one swoop. We have left with 1,005 outstanding defects (this number probably including those from 3rd-party) and brought our defect density down to 0.74.

If you are still interested in having the permission to classify the defects in our coverity scan defects report then you can apply it via an online form provided in their web interface. Once I receive your request, I will be gladly to approve it.

@ricab

This comment has been minimized.

Copy link
Contributor Author

commented Jan 28, 2018

Great, that is some improvement :)

I just tried classifying a couple of issues and it seemed to work. Could you confirm you see it? e.g. issue 216066 (hope the link works).

Please notice that some of this stuff is subjective enough to engage in whole holy discussions (RAII, premature optimization, etc.). So let me know if the approach I took in that case is welcome or not - I could write a TrailPoint ctor or just leave it be.

@weitjong

This comment has been minimized.

Copy link
Member

commented Jan 29, 2018

I can see that CID has been classified by you. Just check the member list, you have been accepted as the last member since May 15 2017 😄

About the false positive. Yeah, that is the main reason why we had sort of abandoning it after we successfully set it up. Although once in a while the scan report does reveal genuine defects. So we still want it to make it to become useful some day. Having someone to help to classify remaining outstanding issues will be awesome.

About the "uninitialized variable" issue, we do get that as the lint error from clang-tidy as well if version 5 or 6 is being used. As I mentioned before somewhere else, I have a plan to upgrade the current clang-tidy version to the latest version 6 (as Clang 6 release is imminent). So, we have to deal with it one way or another.

@weitjong

This comment has been minimized.

Copy link
Member

commented Jan 30, 2018

If you are serious about helping us to classify and fixing the genuine defects then I suggest you raise another new issue in our issue tracker to keep track of your progress and to make your work more visible. And when/if you could generate PRs faster than we could review/merge them then we can also grant you the push privilege directly.

@ricab

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2018

Yes, well, I can't devote much time to it, but I was thinking of doing a couple once in a while, on the assumption that little is better than nothing 😁 I am about to submit a PR with a fix for 4 issues in RibbonTrail.cpp. Let me open that issue as you suggest then.

Having a pair of eyes on PRs is useful for the moment indeed, at least until I come to grips with the formatting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.