Page MenuHomePhabricator

Added Attribute Encoder
AbandonedPublic

Authored by ldmouse on Jan 4 2016, 6:15 PM.
This revision can not be accepted until the required legal agreements have been signed.

Details

Reviewers
beanders
jcmcdonald
Required Signatures
Restricted Legalpad Document
Summary

Commit to preserve my attribute encoder work thus far. (Did not debug yet.)

Test Plan

tbd

Diff Detail

Repository
rP PawLIB
Branch
attributes_branch
Lint
Lint Errors

Event Timeline

beanders retitled this revision from to Added attribute encoder but did not debuug it yet.
beanders updated this object.
beanders edited the test plan for this revision. (Show Details)
  • Compilining but not debuged version after Reference refactor and some other pointer removal
  • Compilining but not debuged version after Reference refactor and some other pointer removal
  • Primitive Encode/Decode test is working again
  • Compilining but not debuged version after Reference refactor and some other pointer removal
  • Primitive and SVGDataType tests are working again. Added comments to AttributeEncoderHelper.hpp
jcmcdonald retitled this revision from Added attribute encoder but did not debuug it yet to Added Attribute Encoder.Feb 6 2016, 11:35 AM
jcmcdonald updated this object.
jcmcdonald changed the repository for this revision from rP PawLIB to rPL Punchline.
jcmcdonald added a subscriber: jcmcdonald.

I updated your title for clarity, and realized that this was originally sent up against the PawLIB repository, not the Punchline repository. Are you certain you're working on the Punchline repository? (If so, this may be a bug in Phabricator.)

  • Compilining but not debuged version after Reference refactor and some other pointer removal
  • Primitive and SVGDataType tests are working again. AttributeEncoderHelper tests working through paint. Added comments to AttributeEncoderHelper.hpp
  • Compilining but not debuged version after Reference refactor and some other pointer removal
  • Primitive and SVGDataType tests are working again. AttributeEncoderHelper tests working through paint. Added comments to AttributeEncoderHelper.hpp
  • Added .gitignore
  • Completed transform list debugging and partially completed begin list debugging.

Would you like a code review on this before it gets too much bigger? It doesn't mean that the code has to LAND right away, but some input early on might help you find and fix bugs faster. (If you would, add me as a reviewer from the Action menu above the comment box at the bottom of the Differential.)

  • Compilining but not debuged version after Reference refactor and some other pointer removal
  • Primitive and SVGDataType tests are working again. AttributeEncoderHelper tests working through paint. Added comments to AttributeEncoderHelper.hpp
  • Added .gitignore
  • Completed transform list debugging and partially completed begin list debugging.
  • Completed primary testing of the AttributeEncoderHelper

Just a few thoughts. Every time I look at Punchline, I am amazed at how detailed it is. Seriously, you deserve a medal...but perhaps you'll settle for a token?

I've set aside a dedicated wiki book for Punchline's documentation, and migrated the SVG docs to that. It would probably be beneficial to document the basic structure of Punchline, thereby making it easier to understand what class does what, and how the whole thing works overall. Punchline wiki.

I'll take a more detailed look later. I'm going to have to digest this thing in very small pieces.

SVGBytecode/include/BitsetBasicSVGTypes.hpp
54

Hey Bryan, just a reminder that you don't need to sign comments anymore. That was for the Synfig project, since commenting efforts would overlap.

237–238

Is there a particular need to use RGB here? If not, hexadecimal may actually be easier to store, as it is just the RGB (or RGBA) values stored in hexadecimal form. Thus, RGB 255,255,255 is just 0xFFFFFF. The other possible advantage might be that a single hex value would have less overhead than a struct.

That said, if there is a particular reason for using RGB in this manner instead, just disregard me. :3

SVGBytecode/include/BitsetSVGAttributeTypes.hpp
280

I'm sure this is correct, but I'm curious now - if a unary value can only have one possible value, what is the purpose behind having unary values? What's the difference from constants? Pardon my density. :)

SVGBytecode/include/Decoding/AttributeDecoder.hpp
5

I'm wondering if you could explain your inheritance structure to me. I see that you have this base abstract class from which you inherit your other AttributeDecoders. I'm sure there's some solid reasoning behind this, but I'd like to see that in comments at the top of this class. What was your logic behind this design decision?

SVGBytecode/include/Util/SVGBytecodeMapCreator.hpp
19

AFAIK, it's generally a good idea to always have at least an empty default constructor and destructor. I usually use the structure...

Default Constructor/Destructor
MyClass(){}
~MyClass(){}

Because I've defined them as empty in the header, I don't have to take up room in the .cpp for them unnecessarily.

SVGBytecode/src/Decoding/BitsetSVGDataTypeDecoder.cpp
104

So, the alpha component was removed? Entirely, or just from this section? The alpha value is very important in colors for Anari.

SVGBytecode/src/Encoding/BitsetAttributeEncoder.cpp
447

Anari will probably replace this functionality altogether, but I'm not certain, so let's leave it in. Anyway, Anari isn't the only platform that'll use Punchline, no doubt.

Responding to Jason's comments. I think I will be continuing to update this diff until I am ready to submit a final AttributeEncoder diff once I test it with my new test file.

SVGBytecode/include/BitsetBasicSVGTypes.hpp
54

I've stopped this in the last few months, but I haven't removed the old ones yet.

237–238

I'll be converting RGB to simply use an int using hex values.

SVGBytecode/include/BitsetSVGAttributeTypes.hpp
280

In this case it looks like an attribute that has the potential to be expanded to have multiple values in the future, but currently has only one. Not sure why they included it at this juncture.

xlink:type

SVGBytecode/include/Decoding/AttributeDecoder.hpp
5

The idea behind the interfaces/inheritance structure was to create a framework that allows for multiple implementations down the road since this will be an open source project. The way the decoders are currently returning void* is kind of annoying, but I'm planning to wrap the decoders with a class that converts and returns the actual type, instead of having to cast all the time inside the project.

SVGBytecode/include/Util/SVGBytecodeMapCreator.hpp
19

Adding this.

SVGBytecode/src/Decoding/BitsetSVGDataTypeDecoder.cpp
104

I changed this after taking a closer look at the SVG color specification here. SVG only defines color as RGB, not RGBA. Opacity is handled via the opacity attribute.

One more comment for you. I really wish I could print off this code and work with it on paper - I always work better on paper. I'll be doing the second best thing and pulling this down onto my computer. It's gonna take me a while to fully review it, though.

SVGBytecode/src/Util/AttributeEncoderHelper.cpp
73

Just reading this, I'm somewhat confused - if the string ends in em, how can it also end in matrix(? Furthermore, wouldn't that be at the start of the string, instead of the end?

beanders marked 6 inline comments as done.
beanders edited edge metadata.
  • Compilining but not debuged version after Reference refactor and some other pointer removal
  • Primitive and SVGDataType tests are working again. AttributeEncoderHelper tests working through paint. Added comments to AttributeEncoderHelper.hpp
  • Added .gitignore
  • Completed transform list debugging and partially completed begin list debugging.
  • Completed primary testing of the AttributeEncoderHelper
  • Initial attribute debugging complete
jcmcdonald edited edge metadata.

I finally did it! I got through the entire code! I wound up just sitting down and wading through the whole thing. I think I understand about 2/3 of the implementation right now, and more will make sense as time goes on. Plus, because I finally have read through this at least once, I can focus on new changes in future revisions.

I hope my comments are helpful. Let me know if you want feedback on a specific portion of the code, and I'll take another look.

SVGBytecode/include/BitsetSVGAttributeTypes.hpp
1

In looking at the last file and this one, it would be massively helpful to have header comments. You can see this at work in PawLIB (for example, see pawsort.hpp). The header comment states the general intent of the document, as well as author and update information.

The license is also present in a separate comment, so it can be collapsed out of the way. You can copy the MIT License straight out of PawLIB.

When you get the chance, please add these header comments to each of your header files. Implementation files do not need header comments, as they are exclusively paired with their header file. The only time an implementation file would need a header comment is if it were a standalone (such as main.cpp).

39

Emum or Enum?

57

In my code, I actually have a typedef to std::string. This means that I can swap all of my strings out to PawString at a later date with a single line of code. Scott is also looking into having two forms of PawString - ASCII and Unicode - so that ASCII-only situations can benefit from PawString's algorithmic performance which would otherwise be offset entirely by the Unicode functionality.

Anyway, it might be beneficial to do the same thing in Punchline:

typedef std::string string;
//...
string strValue;
284

One of my other primary concerns in this document is that there isn't a lot of CSI commenting. Remember, a CSI comment states, not "what", but "why". It's intended to communicate your design intentions and thoughts to other programmers, which is especially important once another coder is solely responsible for maintenance!

An example of a CSI-comment here might be as simple as...

// Define all of our real-number attributes as aliases of our real number attribute, to make the code more readable.

In the larger context of this document, you would write that sort of comment in the first batch of typedefs in which you are aliasing a type as its related attributes. Then, you can get away with much simpler comments from then on.

// Define boolean attributes as aliases.
521

I do have to say, the idea of typedefing attribute names for types is brilliant, as it makes the code considerably easier to read and maintain!

SVGBytecode/include/Encoding/BitsetAttributeEncoder.hpp
488

Following up on my earlier comment about the string typedef: after implementing that typedef, you'll want to do a find-and-replace for std::string, ensuring that you only have one explicit reference to it - in your typedef. Everywhere else should just use string. Again, this lets you swap string classes out much more quickly, and with less errors.

593–600

I hate to say it, but someone - you or another developer - is going to have to write doc comments for each of these (and all of the other functions).

SVGBytecode/include/Util/AttributeEncoderHelper.hpp
19

These are good starts on doc comments, but it will be a little more readable in this form...

/** Checks if a string matches a given type.
 * \param the string to check
 * \param the type we want the string to match
 * \return true if the string matches type, else false
 */

However, this is a little less critical than other parts of Punchline, so rewriting these can be a low priority, or even left to another coder, since the information is all contained in the existing comments.

90

Honestly, most of these should continue to work after the switch to Pawstring. The aim is for the API to be almost identical to std::string. However, some functionality might be added based on feature requests, if there are any you want to make! (Use the Feature Rest form on Maniphest.)

SIMPLEXpress may also be helpful later, but that's a ways off.

SVGBytecode/include/Util/BitsetGenerator.hpp
139

I've been thinking about this for a while now - at some point, it would be a good idea to go through and remove all of the instances of -BA. In our own code base, we don't need to sign comments, and they're a bit of an "eye-trip" for me (and thus, probably for some other people). That should be a pretty trivial task in Code::Blocks or Geany.

SVGBytecode/src/Encoding/BitsetAttributeEncoder.cpp
285–288

I just figured out that these are living here. While I generally put the doc comments in the header (that's the standard convention), if they work better here, that's fine too.

2296

You should probably say ///TODO: Come back to this. That way, the CodeBlocks TODO List plugin can find it later, and it is less likely to get lost. :)

2475

SAOD (Severe Acronym Overload Disorder). What's an IRI? Within a single code file, you want to write out the acronym (if possible) first, with the acronym itself following in brackets, like this...

// Use the Super Fantastic Number Processor [SFNP]
// ...lots of code...
// Use the SFNP

An obvious exception for this is when you're referring to a technology [XML], organization [GNU], library [GTK], or widespread term [CPU] by its common acronym name.

2548
WARNING: This may not work right off the bat with Scott's PawString, especially given the std::string::npos. You may need to file a feature request if you don't want to have to re-hardcode this later.
4020

This is rather pedantic, but ++i is one instruction less than i++ (before compiler optimization), and thus preferable for most for loops. However, the compiler automatically optimizes i++ and ++i in this context, so while it's a good habit to use the prefix, it isn't strictly necessary. Like I said, it's pedantic. :)

Macro _lawyerdog_: Language lawyer is pedantic.

4063

///TODO: This needs some work.

SVGBytecode/src/Encoding/BitsetSVGDataTypeEncoder.cpp
38–41

Would it be helpful to make sure data is not null_ptr?

SVGBytecode/src/Util/AttributeEncoderHelper.cpp
10

Should this be a NOTE or a TODO?

615–616

Just a random thought - once Punchline is hooked up to PawLIB, would it be helpful to actually display a "Not Implemented" warning message to this effect?

1088

When I first read delLoc, I thought it meant "delete lock", as del is a pretty common abbreviation for delete. You may want to consider renaming this for clarity.

1278–1279

If you need/want to get rid of this sooner, I have a split function that doesn't use std::stringstream. See Q14: Split std::string in C++

SVGBytecode/src/Util/BitsetGenerator.cpp
2–16

We updated convention to split the LICENSE off into its own comment. See my first inline comment in this review for more details.

SVGBytecode/src/Util/SVGBytecodeMapCreator.cpp
293

Ouch. That had to be hard code to write! Good job, though.

This revision now requires changes to proceed.May 6 2016, 3:33 PM
jcmcdonald edited reviewers, added: beanders; removed: jcmcdonald.

Commandeering to abandon. This will need to be revisited later if needed.

This revision now requires review to proceed.Nov 20 2019, 3:50 PM
ldmouse added a reviewer: jcmcdonald.
Herald added a required legal document: Restricted Legalpad Document. · View Herald TranscriptNov 20 2019, 4:41 PM

Content Hidden

The content of this revision is hidden until the author has signed all of the required legal agreements.