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

Adding attributes to tags in svg backend #72

Open
fcooper8472 opened this issue Jul 13, 2018 · 3 comments
Open

Adding attributes to tags in svg backend #72

fcooper8472 opened this issue Jul 13, 2018 · 3 comments
Labels
enhancement New feature or request

Comments

@fcooper8472
Copy link
Collaborator

I would like to implement some helper to make this kind of thing look less nasty (easier to read, reason about, and maintain):

m_out << "x=\"" << min[0] << "\" y=\"" << min[1] << "\" width=\"" << delta[0] << "\" height=\"" << delta[1] << "\" ";

There are several possible options.

1. Function template, something like:

template<typename T>
std::string att(const std::string& name, const T val)
{
  std::stringstream ss;
  ss << std::setprecision(5) << name << "=\"" << val << "\" ";
  return ss.str();
}

so that the above example becomes

m_out << att("x", min[0]) << att("y", min[1]) << att("width", delta[0]) << att("height", delta[1]); 

This looks quite nice, and is certainly much easier to read. But, the overhead of generating lots of stringstream objects and returning strings is high (~2x slower). Speed probably isn't a massive concern at this stage, but let's avoid massive slowdowns if there's a better option.

2. As above, but sending straight to m_out:

I don't like this option at all, because the example becomes:

att("x", min[0]); att("y", min[1]); att("width", delta[0]); att("height", delta[1]); 

It's super unclear at a glance what's happening and doesn't have any flexibility at all.

3. A small class for gathering attributes:

struct attribs {
  std::stringstream ss;
  attribs& add(const std::string& name, const float val) {
    ss << std::setprecision(5) << name << "=\"" << val << "\" ";
    return *this;
  }
  /* overload << */
};

The example becomes

attribs att;
att.add("x", min[0]).add("y", min[1]).add("width", delta[0]).add("height", delta[1]);
m_out << att;

This has a greatly reduced cost vs 1, is flexible, but perhaps not the most elegant.

@martinjrobins Any other ideas for how this might look?

@martinjrobins
Copy link
Collaborator

martinjrobins commented Jul 14, 2018

  1. looks the best out of those options, here are a couple of suggestions to increase speed, would any of these work?:
  • using snprintf to write to a char buffer, then just return that buffer. This would definitely be faster but perhaps not as safe
  • write it as a function object, perhaps if you re-use the stringstream object it will be faster?:
struct att {
std::stringstream ss;

att() {  ss << std::setprecision(5); }

template<typename T>
std::string operator()(const std::string& name, const T val)
{
  ss.str("");
  ss << name << "=\"" << val << "\" ";
  return ss.str();
}
};

@fcooper8472
Copy link
Collaborator Author

@martinjrobins your struct is actually slightly faster than mine, with nicer semantics too 👍.

Using snprintf is a fair bit faster still:

xigxzblxankjfav4jp5a4f2dray

But, I really don't like it as a solution:

  • It would be a pain to dynamically alter precision
  • It wouldn't work for custom arguments with a << operator, like RGBA

I think I'll go ahead and implement something like your suggestion and we can see what it looks like.

@martinjrobins
Copy link
Collaborator

I agree, I don't think using snprintf is worth the extra speed.

fcooper8472 pushed a commit to fcooper8472/trase that referenced this issue Jul 14, 2018
@martinjrobins martinjrobins added the enhancement New feature or request label Oct 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants