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

API & Framework changes #74

Merged
merged 61 commits into from
Mar 25, 2016
Merged

API & Framework changes #74

merged 61 commits into from
Mar 25, 2016

Conversation

9prady9
Copy link
Member

@9prady9 9prady9 commented Jan 26, 2016

Includes the following:

In Progress:
  * Chart class porting

Finished:
  * GLSL shader to std::string headers
  * Ported Image class to new framework,
    image examples working fine
Debugging following issues:
  * histogram rendering seems to be buggy
  * 3d line plot and surface are not rendering anything, though
    the 3d charts by themselves are rendering fine.

Finished:
  * GLSL shader to std::string headers
  * Ported Image class to new framework, image examples working fine
  * 2d line and scatter plots working fine, multiple plots per chart
    also working fine.
  * Ported histogram, 3d line plot and histogram to new framework.
Debugging following issues:
  * 3d line plot and surface are not rendering anything, though
    the 3d charts by themselves are rendering fine.

Finished:
  * GLSL shader to std::string headers
  * Ported Image class to new framework, image examples working fine
  * 2d line and scatter plots working fine, multiple plots per chart
    also working fine.
  * Ported histogram, and corresponding example works fine
  * 3d line plot and surface to new framework, 3d scatter plot is
    working fine.
To do:
  * Enable alpha blending in all renerables, shader code and
    required code is mostly already in place, have to add
    blending behavior
  * Add legends to the renderings
  * Add interactivity controls for renderables
  * Add event handling to control flow

Finished:
  * Automated conversion of GLSL shader files to std::string headers
  * Ported all renderable objects to new framework that supports
    multiple renderings per Chart.
  * Moified examples to use modified API
Also, updated documentation for other classes
* Translate - Left mouse click + drag
* Zoom      - Left mouse click + drag with ALT (either left or right) modifier
* Rotate    - RIght mouse click + drag

Both glfw3 and SDL2 windowing toolkits support the above three
interactions.

TODO:
* Limit transformations to the view in which the cursor is present for
  multiview rendering mode.
* Make rotations more robust, probably quaternion based, probably virtual
  track ball rotation is good for plots/graphs
@9prady9 9prady9 added this to the 1.0.0 milestone Jan 26, 2016
Modified histogram cpu, cuda & OpenCL to show case per vertex/primitive
colors feature

Similar examples to show case alpha blending for plots will be added
soon.
Also, moved Y axis label to the right side to have consistent
position for the label irrespective of the tick labels displayed.
X axis tick labels are centered around the decimal point now.
Also modified COPY helper headers to be more generic where the
copy functions directly take OpenGL buffer object identifiers
and the corresponding buffer size in bytes instead of Forge objects.
* Fixes in fragment shaders
* Disabled depth test for alpha blending
this example also demonstrates transparency and multiple plot rendering
* Removed font size parameter from loadFont functions
* Font class loads a range of font sizes a prior
* Characters have an outline to enable easy reading
* Updated examples to reflect the change related to loadFont functions
* Added a new FontAtlas class that creates the single texture that
  has all the glphs for all font sizes and characters

Commented out outline strokes currently, will enable them after
further testing.
@shehzan10
Copy link
Member

Since we are adding freeimage, any thoughts on having a function that loads and displays the image in a single call?

@shehzan10
Copy link
Member

I notice that the C++ classes do not have constructors that accept the c-style handles. I think this should be available.

@shehzan10
Copy link
Member

I think font management should be a manager. There should be a map between the fonts loaded and the handle. If there is a constant loading of different fonts, that can be a big issue.

@shehzan10
Copy link
Member

General concerns/clarifications:

  • The fg_* display typedefs (window, chart etc), are all typedefs to void*. I'm a little concerned about how much they'll be abused.
  • The getDisplay(fg_display *) functions do not check if the inputs are valid pointers. I think this can be resolved if we have a display manager of sorts that handles the pointers etc.
  • A lot of functions such as set positions, axes, limits, colors etc are not being checked for value ranges. Is there merit in checking these?
  • Any reason why we are implementing the src/backend/*.hpp classes in the header files?
  • I don't like having 2 different typedefs for uint etc in namespace fg and internal. Might as well typedef internal names to the fg ones, ie typedef fg::type internal::type

Edited by Pradeep in response to above queries:

  • I originally thought of modifying thecommon::* classes into struct's and using the pointers to those structures as handles. But doing so would require us to expose the definition of those structures to users(Keep in mind these structures(currently classes) having a single member variable - shared pointer). Therefore, i didn't think it would work
  • I am assuming you are referring to functions such as getWindow(fg_window) etc. I think it is better to avoid a global class if we can and in this case, we don't need one. These functions are just beautifying the reinterpret_cast similar to the way getHandle/getArray does in ArrayFire.
  • we can definitely add some very basic checks, like you said - i am not sure how helpful they would be. May be the users will appreciate such checks.
  • Unlike ArrayFire, forge has three layers naturally from the beginning due to manage the objects of classes that encapsulate OpenGL resources. Right now, all opengl implementation classes are in opengl(backend) namespace. The classes in src/backend/*.hpp have shared pointers to the the implementation classes in backend namespace. These are minimalistic classes that delegate function calls to the actual objects(implementation) in backend. Splitting the one line implementations of these delegation classes into source files and header files is not going to help us in any fashion other than in increasing the LOC.
  • hmm, I am not sure if you pulled the latest changes in this branch. There is no internal namepace anymore. Also, no such different typesdefs. If you still find them in latest commit, then i must have missed removing them. Please tell m where you found them.

@9prady9
Copy link
Member Author

9prady9 commented Mar 8, 2016

Replying here since original comment is cluttered with others above.

Regarding the following comments by @shehzan10

I notice that the C++ classes do not have constructors that accept the c-style 
handles. I think this should be available.

Sure we can add them, makes the code in api files more terse.

Since we are adding freeimage, any thoughts on having a function that loads 
and displays the image in a single call?

Forge so far has been object centric, you need a window to render something. Have a function such as below

displaImage(...)

would require us to keep state (internally maintain a Window) to display any image that gets passed into the above function.

@shehzan10
Copy link
Member

I'm not onboard with implementing classes in the backend/*.hpp files in the header, especially the constructors.
Also, I've noticed most of them (histogram, plot, chat etc) return the same member variables etc in the function. Why not just use a derived class? All of them seem to have very similar members.

@9prady9
Copy link
Member Author

9prady9 commented Mar 8, 2016

@shehzan10 Regarding using inheritance at the common namespace level for renderable objects, didn't occur to me until you pointed it out. Let me see how we can do it with least code duplication. If you have already thought it out, please post it here.

@shehzan10
Copy link
Member

I haven't thought of a specific design. But you can essentially put all the common stuff in the interface and then derive that. I guess ArrayInfo or MemoryManager classes in ArrayFire are a good place to look.

Proper perlin noise is being displayed and the corresponding gaussian
distribution of the noise on the right.
@9prady9
Copy link
Member Author

9prady9 commented Mar 24, 2016

@pavanky @shehzan10 Okay, i fixed the cuda and opencl examples. Please run them on your machines and let me know if any issues are there. I will move to trying glbinding now.

typedef unsigned short ushort;
typedef unsigned char uchar;

typedef enum {
FG_SUCCESS = 0, ///< Fuction returned successfully.
Copy link
Member

Choose a reason for hiding this comment

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

I like the idea of having FG_ERR_NONE as opposed to FG_SUCCESS (or at least have both) for consistency reasons.

@9prady9
Copy link
Member Author

9prady9 commented Mar 25, 2016

@pavanky Finished types enum changes we have discussed, PR is ready now.

@pavanky pavanky merged commit f5742fc into arrayfire:devel Mar 25, 2016
@9prady9 9prady9 deleted the api_changes branch May 27, 2016 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants