Skip to content

Conversation

theunwisewolf
Copy link
Contributor

@theunwisewolf theunwisewolf commented Aug 11, 2022

THIS PROJECT IS IN DEVELOPMENT MODE. Any pull requests should merge to branch dev, otherwise will be rejected immediately.

Describe your changes

Added support for "scaleX", "scaleY" and "scale" tags for rich text tag.
Added support for width/height in percentage. E.g. <img width="150%" />
Also fixed a bug with the url attribute. Even if you didn't put a url in your img, it will still call openUrl() for some reason.

Checklist before requesting a review

  • I have performed a self-review of my code
  • If it is a core feature, I have added thorough tests.

@halx99
Copy link
Collaborator

halx99 commented Aug 14, 2022

What about implement html standard width="150%"

@theunwisewolf
Copy link
Contributor Author

Check @halx99

@@ -471,13 +471,29 @@ MyXMLVisitor::MyXMLVisitor(RichText* richText) : _fontElements(20), _richText(ri
it = tagAttrValueMap.find("height");
if (it != tagAttrValueMap.end())
{
height = it->second.asInt();
auto str = it->second.asStringRef();
if (str[str.length() - 1] == '%')
Copy link
Contributor

@rh101 rh101 Aug 16, 2022

Choose a reason for hiding this comment

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

It's best to check if the string is empty before attempting to do a [length - 1], because this will definitely cause a crash if the string is like this: height="". Same goes for the width.

You could also use this instead if (str.back() == '%'), but you would still need to check if the string is empty beforehand.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then again, there doesn't seem to be much (if any) validation code in the RichText implementation for all the other fields either, so perhaps the assumption is that they will always be valid. This could lead to problems if the text is added dynamically (such as based on user-input), but if the text is hard-coded, then any issues will most likely show up in development.

Copy link
Collaborator

@halx99 halx99 Aug 16, 2022

Choose a reason for hiding this comment

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

And all find and try get value could use: ax::optValue(valueMap, "key"sv).asXXX instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rh101, Yeah we can add empty check but as you said, there is no validation anyways so I didn't think it was needed. I will add it.

@halx99 optValue is extremely slow. You should profile it. I have already removed it from my fork as it was slowing down plist loading for particle effects as well as spriteframes. I suspect it has something to do with the custom string_hash & equal_to functions you have written for string_view for use with robin_map (to support both string & string_view interchangeably, right?), although I was not able to figure out the exact cause.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Check does the latest commit solve the performance issue:
d8c9a88

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. I will profile and get back to you.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool. I will profile and get back to you.

Maybe the Optick profiler #845 is helping you?

Copy link
Contributor

@aismann aismann Oct 2, 2022

Choose a reason for hiding this comment

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

Check does the latest commit solve the performance issue: d8c9a88

@halx99
Is this the solution?
So merge can be done or what is missing?

At least this should be fixed:
Also fixed a bug with the url attribute. Even if you didn't put a url in your img, it will still call openUrl() for some reason.

@halx99 halx99 merged commit 92e5f46 into axmolengine:dev Oct 4, 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.

4 participants