Page MenuHomePhabricator

The punchline encoding framework is compiling and running.
ClosedPublic

Authored by beanders on Jul 25 2015, 4:20 PM.

Diff Detail

Repository
rPL Punchline
Lint
Lint Not Applicable

Event Timeline

beanders retitled this revision from to The punchline encoding framework is compiling and running..
beanders updated this object.
beanders edited the test plan for this revision. (Show Details)
beanders added a reviewer: jcmcdonald.
jcmcdonald edited edge metadata.

Don't forget your doc comments on AttributeEncoder.hpp and elsewhere, listing params and returns. Here's the basic structure...

/**Description (you might be able to leave this off to save time).
\param Some parameter.
\param Some other parameter.
\return Return something or other.
*/

Good work so far!

SVGBytecode/notes.txt
5

This should honestly be a Maniphest task. Don't be afraid of making those as you need them.

This revision now requires changes to proceed.Jul 27 2015, 4:52 PM
beanders edited edge metadata.
  • Updated comments, implemented the void based methods interface for the element and attribute encoders and the interperter. Modified the bitsets to use staticly sized integers and created the basic SVG data type definitions.

See the insanely long title

jcmcdonald edited edge metadata.

Very good work so far. Please look through my comments and make the requested changes. In many cases, you'll need to check your code to see where else the indicated change needs to be made.

Punchline is really coming together! Great job.

SVGBytecode/include/BitsetAttributeEncoder.hpp
263 ↗(On Diff #227)

I have to ask- is the media attribute related to sound and/or video playback? If so, it may need to be dropped, as it probably won't translate into Anari.

SVGBytecode/include/BitsetElementEncoder.hpp
158 ↗(On Diff #227)

Is this meant to denote script, as in code? If so, this is another element I'm not sure we want to including because it will not translate to Anari.

SVGBytecode/include/BitsetGenerator.hpp
65 ↗(On Diff #227)

These are EXCELLENT! Good design.

SVGBytecode/src/BitsetElementEncoder.cpp
123 ↗(On Diff #227)
WARNING: Your switch statement is not going to work as expected. Each case's code MUST end with a break;, or it will "fall through" and run all the rest of the code in the other cases. Furthermore, each case's code needs to be wrapped in { }.
switch(bytecode)
{
   case A_BYTECODE:
   {
      encodeElementA();
      break;
   }
   case ALTGLYTH_BYTECODE:
   {
      encodeElementAltGlyph();
      break;
   }
}

Don't feel too bad about it. This threw me for a loop for the LONGEST time, since AS3 doesn't require the { }.

Be sure to check each of your switch statements in your code! I may have missed some.

463 ↗(On Diff #227)

I kept meaning to let you know...you don't strictly have to sign your comments in company code. I only required it in the Synfig code to keep track of our comments vs. their comments, and help in case one person's commenting path crossed another person's.

SVGBytecode/src/BitsetGenerator.cpp
63 ↗(On Diff #227)

The proper way to do this, so you get the advantage of the doc comments in the autocomplete, is as follows...

/** Creates an array of bitsets with as many elements
as a string has chars and initializes each using the corresponding char.
\param string to convert into char bitsets
\return an array of CharBitset pointers
**/

Note that `\param` and `\return` both start with a backslash (`\`), and the entire thing is wrapped in doc-style comment (`/**` and `*/`)
SVGBytecode/src/SVGImportErrorHandler.cpp
7 ↗(On Diff #227)

The license is either LGPL v3 or MIT. We can discuss on Wednesday which you want to use.

This revision now requires changes to proceed.Aug 3 2015, 5:52 PM

I'll make a list of elements/attributes that do not need to be encoded/decoded for Punchline. They can simply have empty implementations.

EDIT: How do I set uncommitted changes within this page to comitted?

beanders edited edge metadata.
beanders marked 5 inline comments as done.
  • Implemented changes specified in the previos git revision
jcmcdonald edited edge metadata.

Just a few adjustments on the comments. Sorry to keep bugging you with that.

The biggest thing is that doc comments for a function, with the \param and \return stuff, need to be a single comment /** */, not several /// lines, for them to be parsed correctly.

Good work so far!

SVGBytecode/src/BitsetBytecodeEncodingInterpreter.cpp
49 ↗(On Diff #230)

You probably did find/replace from return to \return, as this isn't needed here (it's not a doc comment). However, it's not a big deal. :3

SVGBytecode/src/BitsetElementEncoder.cpp
621 ↗(On Diff #230)

Good job on the fix. =)

SVGBytecode/src/BitsetGenerator.cpp
2 ↗(On Diff #230)

Did you mean /**, instead of /*s?

40 ↗(On Diff #230)

Doc comments of this style only work with /** */ around the whole thing, AFAIK.

SVGBytecode/src/SVGImporter.cpp
7 ↗(On Diff #230)

Typo.

This revision now requires changes to proceed.Aug 8 2015, 2:51 PM
beanders edited edge metadata.
beanders marked 4 inline comments as done.
  • Fixed comments for parsing and other small diff errors. I thought I was being lazily clever by changing my single line function comments into a couple ///. I was wrong :p
jcmcdonald edited edge metadata.

Bravo! Excellent work, Bryan.

This revision is now accepted and ready to land.Aug 10 2015, 3:53 PM
This revision was automatically updated to reflect the committed changes.

Pushed.

SVGBytecode/include/BitsetAttributeEncoder.hpp
263 ↗(On Diff #227)

Media denotes one or a list of css media descriptors described here. Would any of them be useful?

SVGBytecode/include/BitsetElementEncoder.hpp
158 ↗(On Diff #227)

It is script as in the html script tag.

<script type="text/javascript">
    // <![CDATA[
    function change(evt) {
      var target = evt.target;
      var radius = target.getAttribute("r");

      if (radius == 15) {
        radius = 45;
      } else {
        radius = 15;
      }

      target.setAttribute("r",radius);
   }
   // ]]>
  </script>
SVGBytecode/src/BitsetElementEncoder.cpp
463 ↗(On Diff #227)

Oh, ok. I'll probably keep signing them. Its easy when I'm already writing and lets people know who to ask about questions.

SVGBytecode/src/BitsetGenerator.cpp
40 ↗(On Diff #230)

And here I thought I was being clever :p

SVGBytecode/src/SVGImportErrorHandler.cpp
7 ↗(On Diff #227)

I think I would like to use MIT.