Skip to content

Resolution for issue Fill or stroke attributes on a use element #162

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.

Sign up for GitHub

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

Open
tjindal wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Resolution for issue Fill or stroke attributes on a use element #162

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4a4bed1
Resolution for issue Fill or stroke attributes on a use element, chan…
Nov 15, 2021
3fd71c3
modified changes for Resolution for issue Fill or stroke attributes o…
Nov 26, 2021
4fdfbaf
removing unused spaces or reverting unwanted spaces
tusharjinda Nov 29, 2021
18ac44d
removes unused spaces
tusharjinda Nov 29, 2021
3cf6720
removes unused spaces
tusharjinda Nov 29, 2021
01c6b0a
changes done for PR #162 suggested by dirk
tusharjinda Dec 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions svgnative/src/SVGDocumentImpl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1070,6 +1070,24 @@ void SVGDocumentImpl::TraverseTree(const ColorMap& colorMap, const Element& elem
{
ApplyCSSStyle(reference.classNames, graphicStyle, fillStyle, strokeStyle);
auto saveRestore = SaveRestoreHelper{mRenderer, reference.graphicStyle};
if ((*(refIt->second)).Type() == ElementType::kGraphic)
{
Graphic& graphic = static_cast<Graphic&>(*(refIt->second));
// it will call assignment operator and pass fillStyle from reference to graphic
graphic = reference;
}
else if((*(refIt->second)).Type() == ElementType::kGroup)

@dirkschulze dirkschulze Dec 1, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This group handling will create another chain of issues. Especially with inherited properties that you don't handle. It also assumes that you just need to pass style information to the first child. However, there might be multiple nested groups and the styles need to get inherited through those groups as well.

<g id="ref">
    <rect width="100" height="100"/>
</g>
<use xlink:href="#ref" x="120" fill="green"/>

In general, the <use> element is like another group with shifting its content to position x/y. Actually, the shifting should be identical to a translation via transform. The referenced content gets "copied in" here. I am quoting because we do not want to actually copy.

The intention was the following:

  1. We have a reference object and the object that gets referenced by it
  2. The reference object has styling applied the styling gets passed to the object that it references like we do for the group object.
  3. The reference object triggers rendering just like the group does. So it renders the referenced object as normal.... the difference is the style object of the group.

So the code should already work as is. See overall comment.

@tjindal tjindal Dec 2, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks Dirk, i get your point of multiple nested groups if given, and i modified changes recently again to make code more generalised.

{
Group& group = static_cast<Group&>(*(refIt->second));
for (auto& child : group.children)
{
if ((*child).Type() == ElementType::kGraphic)
{
Graphic& graphic = static_cast<Graphic&>(*child);
graphic = reference;
}
}
}
TraverseTree(colorMap, *(refIt->second));
}

Expand Down
85 changes: 71 additions & 14 deletions svgnative/src/SVGDocumentImpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,42 +133,99 @@ class SVGDocumentImpl
ElementType Type() const override { return ElementType::kGroup; }
};

struct Graphic : public Element
struct Reference : public Element
Comment thread
dirkschulze marked this conversation as resolved.
{
Graphic(GraphicStyleImpl& aGraphicStyle, std::set<std::string>& aClasses, FillStyleImpl& aFillStyle, StrokeStyleImpl& aStrokeStyle,
std::shared_ptr<Path> aPath)
Reference(GraphicStyleImpl& aGraphicStyle, std::set<std::string>& aClasses, FillStyleImpl& aFillStyle, StrokeStyleImpl& aStrokeStyle,
std::string aHref)
: Element(aGraphicStyle, aClasses)
, fillStyle{aFillStyle}
, strokeStyle{aStrokeStyle}
, path{std::move(aPath)}
, href{std::move(aHref)}
{
}

FillStyleImpl fillStyle;
StrokeStyleImpl strokeStyle;
std::shared_ptr<Path> path;
std::string href;

ElementType Type() const override { return ElementType::kGraphic; }
ElementType Type() const override { return ElementType::kReference; }
};

struct Reference : public Element
struct Graphic : public Element
{
Reference(GraphicStyleImpl& aGraphicStyle, std::set<std::string>& aClasses, FillStyleImpl& aFillStyle, StrokeStyleImpl& aStrokeStyle,
std::string aHref)
Graphic(GraphicStyleImpl& aGraphicStyle, std::set<std::string>& aClasses, FillStyleImpl& aFillStyle, StrokeStyleImpl& aStrokeStyle,
std::shared_ptr<Path> aPath)
: Element(aGraphicStyle, aClasses)
, fillStyle{aFillStyle}
, strokeStyle{aStrokeStyle}
, href{std::move(aHref)}
, path{std::move(aPath)}
{
}

FillStyleImpl fillStyle;
StrokeStyleImpl strokeStyle;
std::string href;

ElementType Type() const override { return ElementType::kReference; }
};
std::shared_ptr<Path> path;

ElementType Type() const override { return ElementType::kGraphic; }
Graphic& operator= (const Reference& refObj)
{
if (refObj.fillStyle.hasFill)
{
this->fillStyle.hasFill = refObj.fillStyle.hasFill;

@dirkschulze dirkschulze Dec 1, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No space aligning please.

@tjindal tjindal Dec 2, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

in above example you mentioned , by doing recent changes output is showing 'green' which is desired one. As style object of reference object is taken into account by defining assignment operator , which pass on the fill and stroke properties if any from reference object to object that gets referenced.

i have removed the space aligning.

@tjindal tjindal Dec 6, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

 <g id ="q" fill="yellow">
    <rect id="r" width="100" height="100"/>
</g>
<use xlink:href="#r" x="120" />

in above example ,with the original code (not including any recent changes) the output is first rect is yellow and second is also yellow. even our expectation is that the first rect will be yellow and the second is black.
we have a stack of fill and stroke styles. the first rect takes fill style from parent . That FillStyle will be passed as a reference to function “ParseGraphicProperties” as it modifies FillStyle if any fill attribute found.currently in this scenario, it did not able to modify because no fill attribute is present. Then “Graphic” element with its FillStyle is pushed in stack. There is no way to distinguish its parent FillStyle and current one.

when reference("use") element is pointing to any graphic element, simply it will look up into map "mIdToElementMap" and if graphic element is found, it will further draw graphic element. inheritance is broken there after lookup from above map, all styling attributes which are applied to reference object they are ignored.
simply graphic element will be drawn as it was previously stored in stack. so “yellow” rect will be drawn.

i have also debug the area you pointed in discussion
auto saveRestore = SaveRestoreHelper{mRenderer, reference.graphicStyle};

it simple saves the graphic style , but fill and stroke properties are not stored anywhere which actually fills the rect.

@tjindal tjindal Dec 14, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@dirkschulze can you pls share your thoughts on above comment ?

this->fillStyle.internalPaint = refObj.fillStyle.internalPaint;
}
if (refObj.fillStyle.fillRule != WindingRule::kNonZero)
this->fillStyle.fillRule = refObj.fillStyle.fillRule;

if (refObj.fillStyle.fillOpacity != 1.0f)
this->fillStyle.fillOpacity = refObj.fillStyle.fillOpacity;

Color paint = Color{{0, 0, 0, 1.0}};
if (paint != boost::get<Color>(refObj.fillStyle.paint))
this->fillStyle.paint = refObj.fillStyle.paint;

if (refObj.fillStyle.visibility != true)
this->fillStyle.visibility = refObj.fillStyle.visibility;

ColorImpl color = Color{{0.0f, 0.0f, 0.0f, 1.0f}};
if (refObj.fillStyle.color != color)
this->fillStyle.color = refObj.fillStyle.color;

if (refObj.fillStyle.clipRule != WindingRule::kNonZero)
this->fillStyle.clipRule = refObj.fillStyle.clipRule;

if (refObj.strokeStyle.hasStroke)
{
this->strokeStyle.hasStroke = refObj.strokeStyle.hasStroke;
this->strokeStyle.internalPaint = refObj.strokeStyle.internalPaint;
}
if (refObj.strokeStyle.strokeOpacity != 1.0f)
this->strokeStyle.strokeOpacity = refObj.strokeStyle.strokeOpacity;

if (refObj.strokeStyle.lineWidth != 1.0f)
this->strokeStyle.lineWidth = refObj.strokeStyle.lineWidth;

if (refObj.strokeStyle.lineCap != LineCap::kButt)
this->strokeStyle.lineCap = refObj.strokeStyle.lineCap;

if (refObj.strokeStyle.lineJoin != LineJoin::kMiter)
this->strokeStyle.lineJoin = refObj.strokeStyle.lineJoin;

if (refObj.strokeStyle.dashArray.size() != 0)
this->strokeStyle.dashArray = refObj.strokeStyle.dashArray;

if (refObj.strokeStyle.miterLimit != 4.0f)
this->strokeStyle.miterLimit = refObj.strokeStyle.miterLimit;

if (refObj.strokeStyle.dashOffset != 4.0f)
this->strokeStyle.dashOffset = refObj.strokeStyle.dashOffset;

if (paint != boost::get<Color>(refObj.strokeStyle.paint))
this->strokeStyle.paint = refObj.strokeStyle.paint;

return *this;
}
};
SVGDocumentImpl(std::shared_ptr<SVGRenderer> renderer);
~SVGDocumentImpl() {}

Expand Down