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

Ribbon implementation #744

Merged
merged 10 commits into from Oct 28, 2014

Conversation

Projects
None yet
3 participants
@lukaspj
Contributor

lukaspj commented Jul 30, 2014

This PR adds an implementation of Tim Newell's ribbon code, with a "ParticleEmitterNode-like" RibbonNode.

Since receiveing the code from Tim, the following changes has been made:

  • Option for fixed texCoords has been added to prevent "floating" textures.
  • Tilescale has been added to scale texCoords.
  • Now uses RIBBON_NUM_FIELDS instead of constant values for key value arrays.
  • Option for skipping an amount of emitted segments such that you can optimize the ribbons when necessary by emitting e.g. 50% of the original amount of segments, at the cost of less "smoothness".
  • Now only update buffers when new segments are added (Optimization).
  • Option for preventing texture stretching, such that textures always look uniformly sized.
  • A number of fixes, like being able to have more than 2 key-values.

This video shows off most of the new "front-end" features.

Added PCNTT vertex type.
This is a prerequisite change for the Ribbon implementation.
@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Jul 30, 2014

Contributor

I love you. But, it looks like the build failed with this error:

../../../../Engine/source/T3D/fx/ribbonNode.cpp: In member function ‘virtual void RibbonNode::advanceTime(F32)’:
../../../../Engine/source/T3D/fx/ribbonNode.cpp:199:49: error: no matching function for call to ‘Ribbon::addSegmentPoint(Point3F, MatrixF&)’
    mRibbon->addSegmentPoint(getPosition(), trans);
Contributor

crabmusket commented Jul 30, 2014

I love you. But, it looks like the build failed with this error:

../../../../Engine/source/T3D/fx/ribbonNode.cpp: In member function ‘virtual void RibbonNode::advanceTime(F32)’:
../../../../Engine/source/T3D/fx/ribbonNode.cpp:199:49: error: no matching function for call to ‘Ribbon::addSegmentPoint(Point3F, MatrixF&)’
    mRibbon->addSegmentPoint(getPosition(), trans);
@lukaspj

This comment has been minimized.

Show comment
Hide comment
@lukaspj

lukaspj Jul 30, 2014

Contributor

Huh, I compiled and ran it before PR'ing.. I'll have a look at it.

Contributor

lukaspj commented Jul 30, 2014

Huh, I compiled and ran it before PR'ing.. I'll have a look at it.

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Jul 30, 2014

Contributor

The build failed on Ubuntu, but I suspect that's just because that was the build that ran first. If you really can't reproduce it, maybe try compiling a dedicated server.

Contributor

crabmusket commented Jul 30, 2014

The build failed on Ubuntu, but I suspect that's just because that was the build that ran first. If you really can't reproduce it, maybe try compiling a dedicated server.

@lukaspj

This comment has been minimized.

Show comment
Hide comment
@lukaspj

lukaspj Aug 6, 2014

Contributor

Btw don't know if you noticed it but it seems to compile now.

Contributor

lukaspj commented Aug 6, 2014

Btw don't know if you noticed it but it seems to compile now.

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Aug 7, 2014

Contributor

Yup! Just have other things to do for 3.6 :/

Contributor

crabmusket commented Aug 7, 2014

Yup! Just have other things to do for 3.6 :/

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Sep 13, 2014

Contributor

Ideally we'd like some documentation on this for it to be accepted: specifically, docs on each field in RibbonData and an example datablock in a console doc fragment. If you're too busy to do these, I'll take it on myself!

Contributor

crabmusket commented Sep 13, 2014

Ideally we'd like some documentation on this for it to be accepted: specifically, docs on each field in RibbonData and an example datablock in a console doc fragment. If you're too busy to do these, I'll take it on myself!

@lukaspj

This comment has been minimized.

Show comment
Hide comment
@lukaspj

lukaspj Sep 15, 2014

Contributor

Sure thing I'll brew something up.

Contributor

lukaspj commented Sep 15, 2014

Sure thing I'll brew something up.

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Sep 15, 2014

Contributor

I ask because I'd like to get this in 3.6 as eye candy. It should be easy for us to review since it doesn't touch much existing stuff, just adds features.

Contributor

crabmusket commented Sep 15, 2014

I ask because I'd like to get this in 3.6 as eye candy. It should be easy for us to review since it doesn't touch much existing stuff, just adds features.

@lukaspj

This comment has been minimized.

Show comment
Hide comment
@lukaspj

lukaspj Sep 15, 2014

Contributor

I'll write doxygen documentaion for the Ribbon class, and I'll put up datablocks for ribbons with and without textures. Just give me a few days :)

Contributor

lukaspj commented Sep 15, 2014

I'll write doxygen documentaion for the Ribbon class, and I'll put up datablocks for ribbons with and without textures. Just give me a few days :)

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Sep 15, 2014

Contributor

Sweet, thanks!

Contributor

crabmusket commented Sep 15, 2014

Sweet, thanks!

@crabmusket crabmusket referenced this pull request Sep 16, 2014

Closed

Particle ribbon #648

lukaspj added some commits Jul 30, 2014

Ribbon class implementation.
This class is based on Tim Newell from MaxGaming Technologies code, it's a
Ribbon class which is easy to use from other classes.
RibbonNode class.
Simple RibbonNode class for an implementation similar to that of
ParticleEmitterNodes. Datablock currently has no static fields.
@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Sep 19, 2014

Contributor

Reviewed and it looks pretty good. Questions, though:

  • Interpolation? Or should this be handled already?
  • Example art does not belong in Empty, but thanks for the stuff in Full
  • Need to add an entry to the editor GUI to allow these items to be created without opening the console

I'll make a PR to your repo with these changes, so they can be added to this PR without me having to open a new one.

Contributor

crabmusket commented Sep 19, 2014

Reviewed and it looks pretty good. Questions, though:

  • Interpolation? Or should this be handled already?
  • Example art does not belong in Empty, but thanks for the stuff in Full
  • Need to add an entry to the editor GUI to allow these items to be created without opening the console

I'll make a PR to your repo with these changes, so they can be added to this PR without me having to open a new one.

if(!mActive || mRibbon.isNull() || !mDataBlock)
return;
MatrixF trans(getTransform());

This comment has been minimized.

@crabmusket

crabmusket Sep 19, 2014

Contributor

Not getRenderTransform? Then the next line would have to use getRenderPosition.

@crabmusket

crabmusket Sep 19, 2014

Contributor

Not getRenderTransform? Then the next line would have to use getRenderPosition.

This comment has been minimized.

@lukaspj

lukaspj Sep 19, 2014

Contributor

Just implemented it as it is in ParticleEmitterNodes:

getTransform().mulV(emitAxis);
getTransform().getColumn(3, &emitPoint);

Would need to change both in that case imo, though it doesn't really matter.

@lukaspj

lukaspj Sep 19, 2014

Contributor

Just implemented it as it is in ParticleEmitterNodes:

getTransform().mulV(emitAxis);
getTransform().getColumn(3, &emitPoint);

Would need to change both in that case imo, though it doesn't really matter.

This comment has been minimized.

@crabmusket

crabmusket Sep 19, 2014

Contributor

Fair point. I'm just thinking, for example, a ribbon attached to a node on an animating shape. We'd want it to use the render position to account for that animation, surely. It's probably even more important here than in particle emitter because the interpolation between ribbon segments is actually visible, whereas with particle emitter nodes is generally isn't.

@crabmusket

crabmusket Sep 19, 2014

Contributor

Fair point. I'm just thinking, for example, a ribbon attached to a node on an animating shape. We'd want it to use the render position to account for that animation, surely. It's probably even more important here than in particle emitter because the interpolation between ribbon segments is actually visible, whereas with particle emitter nodes is generally isn't.

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Sep 19, 2014

Contributor

Oh, now I see that some of those Empty additions are because of shaders that need to exist. Le sigh. Oh... and they're... specific to the new art? Is there any sort of generic ribbon shader that could be added? What's the minimum amount of script needed for ribbons to work? Please don't tell me I have to write a new shader for every type of ribbon I want?

Contributor

crabmusket commented Sep 19, 2014

Oh, now I see that some of those Empty additions are because of shaders that need to exist. Le sigh. Oh... and they're... specific to the new art? Is there any sort of generic ribbon shader that could be added? What's the minimum amount of script needed for ribbons to work? Please don't tell me I have to write a new shader for every type of ribbon I want?

@lukaspj

This comment has been minimized.

Show comment
Hide comment
@lukaspj

lukaspj Sep 19, 2014

Contributor

What do you mean with interpolation?

The minimal amount of script would be all the "basic" ribbon stuff. It's the simplest ribbon there is (just a simple colored ribbon).

You have to write a shader for specific ribbons yes. But it's no big deal.. E.g. the basic ribbon shader renders a single color, but you can choose the color in the ribbon datablock.
Same goes for the texture ribbon.

I just thought that a simple uniformly colored ribbon and a textured ribbon would be the most common use-cases, so I decided to add both.

Contributor

lukaspj commented Sep 19, 2014

What do you mean with interpolation?

The minimal amount of script would be all the "basic" ribbon stuff. It's the simplest ribbon there is (just a simple colored ribbon).

You have to write a shader for specific ribbons yes. But it's no big deal.. E.g. the basic ribbon shader renders a single color, but you can choose the color in the ribbon datablock.
Same goes for the texture ribbon.

I just thought that a simple uniformly colored ribbon and a textured ribbon would be the most common use-cases, so I decided to add both.

@jamesford42

This comment has been minimized.

Show comment
Hide comment
@jamesford42

jamesford42 Sep 19, 2014

Random note: CustomMaterial rendering doesn't play nicely with others. Eg, does not get fogged, does not receive shadows, etc. Ideally this would use a regular Material to render, then you get all normal material features as well as playing nicely with others.

jamesford42 commented Sep 19, 2014

Random note: CustomMaterial rendering doesn't play nicely with others. Eg, does not get fogged, does not receive shadows, etc. Ideally this would use a regular Material to render, then you get all normal material features as well as playing nicely with others.

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Sep 19, 2014

Contributor

I realised that when I was asking about interpolation it was because I was playing around with ribbons in the editor like in your video, and the basic ribbon has some obvious kinks in the path - but I realised that's because the editor is just calling setTransform a bunch, rather than doing any interpolated motion, so the ribbon itself doesn't get a chance to interpolate and create segments in those intermediate positions.

I'm not the happiest that defining ribbons from scratch requires defining shaders, but ah well. So do lots of things in the engine. If that's the best solution then I'll have to swallow it. It sounds like it also allows ribbons to be more flexible.

Thanks for the heads-up @jamesford42. Why is CustomMaterial necessary in this case, and what would it take to get ribbons to use regular Materials?

Contributor

crabmusket commented Sep 19, 2014

I realised that when I was asking about interpolation it was because I was playing around with ribbons in the editor like in your video, and the basic ribbon has some obvious kinks in the path - but I realised that's because the editor is just calling setTransform a bunch, rather than doing any interpolated motion, so the ribbon itself doesn't get a chance to interpolate and create segments in those intermediate positions.

I'm not the happiest that defining ribbons from scratch requires defining shaders, but ah well. So do lots of things in the engine. If that's the best solution then I'll have to swallow it. It sounds like it also allows ribbons to be more flexible.

Thanks for the heads-up @jamesford42. Why is CustomMaterial necessary in this case, and what would it take to get ribbons to use regular Materials?

@jamesford42

This comment has been minimized.

Show comment
Hide comment
@jamesford42

jamesford42 Sep 19, 2014

The vertex shader and pixel shader are not actually doing anything custom at all. You can probably just swap in a regular material and it might work. Alternately if there is some setup work for materials I am forgetting about, DecalRoad would be a good reference class since it has transparency but uses regular materials (if i remember correctly).

jamesford42 commented Sep 19, 2014

The vertex shader and pixel shader are not actually doing anything custom at all. You can probably just swap in a regular material and it might work. Alternately if there is some setup work for materials I am forgetting about, DecalRoad would be a good reference class since it has transparency but uses regular materials (if i remember correctly).

@lukaspj

This comment has been minimized.

Show comment
Hide comment
@lukaspj

lukaspj Sep 23, 2014

Contributor

I updated with editor integration and simplified scripts. Also removed some of the datablocks from the Empty template, such that Empty only has the simplest available Ribbon.

I couldn't make regular materials work, maybe someone else can step in on that one?

Contributor

lukaspj commented Sep 23, 2014

I updated with editor integration and simplified scripts. Also removed some of the datablocks from the Empty template, such that Empty only has the simplest available Ribbon.

I couldn't make regular materials work, maybe someone else can step in on that one?

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Sep 23, 2014

Contributor

The new Empty scripts look good, but all those singleton names (datablocks, materials) should have capitalised first letters :P. I sent you a PR (lukaspj#1) with some code style changes to make the source look a bit more like the rest of Torque with more spacing and alignment, and removed the #define.

Contributor

crabmusket commented Sep 23, 2014

The new Empty scripts look good, but all those singleton names (datablocks, materials) should have capitalised first letters :P. I sent you a PR (lukaspj#1) with some code style changes to make the source look a bit more like the rest of Torque with more spacing and alignment, and removed the #define.

Merge pull request #1 from eightyeight/ribbons2
Improved code style in ribbon files
@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Sep 23, 2014

Contributor

Whoops. I went to change the capitalisation of the names but then you merged. I also meant to change texRibbon to TexturedRibbon.

Contributor

crabmusket commented Sep 23, 2014

Whoops. I went to change the capitalisation of the names but then you merged. I also meant to change texRibbon to TexturedRibbon.

@lukaspj

This comment has been minimized.

Show comment
Hide comment
@lukaspj

lukaspj Sep 23, 2014

Contributor

Ah I thought for some reason that you wanted to do it in a different PR...
Re-open the PR with added changes if you can, I'll try and fix it then.

Sincerely
Lukas Joergensen
WinterLeaf Entertainment L. L. C.

-----Oprindelig meddelelse-----
Fra: "Daniel Buckmaster" notifications@github.com
Sendt: ‎24-‎09-‎2014 00:17
Til: "GarageGames/Torque3D" Torque3D@noreply.github.com
Cc: "Lukas Joergensen" ljorgensen@winterleafentertainment.com
Emne: Re: [Torque3D] Ribbon implementation (#744)

Whoops. I went to change the capitalisation of the names but then you merged.

Reply to this email directly or view it on GitHub.=

Contributor

lukaspj commented Sep 23, 2014

Ah I thought for some reason that you wanted to do it in a different PR...
Re-open the PR with added changes if you can, I'll try and fix it then.

Sincerely
Lukas Joergensen
WinterLeaf Entertainment L. L. C.

-----Oprindelig meddelelse-----
Fra: "Daniel Buckmaster" notifications@github.com
Sendt: ‎24-‎09-‎2014 00:17
Til: "GarageGames/Torque3D" Torque3D@noreply.github.com
Cc: "Lukas Joergensen" ljorgensen@winterleafentertainment.com
Emne: Re: [Torque3D] Ribbon implementation (#744)

Whoops. I went to change the capitalisation of the names but then you merged.

Reply to this email directly or view it on GitHub.=

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Sep 23, 2014

Contributor

I'll make a new one. lukaspj#2

Contributor

crabmusket commented Sep 23, 2014

I'll make a new one. lukaspj#2

@crabmusket crabmusket added this to the 3.7 milestone Oct 11, 2014

unkown and others added some commits Oct 21, 2014

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Oct 25, 2014

Contributor

For now, we will merge this using CustomMaterials and add a new issue to investigate whether we should try to modify shadergen so we can use regular mats.

Contributor

crabmusket commented Oct 25, 2014

For now, we will merge this using CustomMaterials and add a new issue to investigate whether we should try to modify shadergen so we can use regular mats.

crabmusket added a commit that referenced this pull request Oct 28, 2014

@crabmusket crabmusket merged commit ef9bc91 into GarageGames:development Oct 28, 2014

1 check passed

default Merged build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment