I would vote for option b) Use a full class that can be extended later as needed. You don't have to use it like an enum. It could be a container for configuration parameters, possibly including the enum that says the antialiasing mode, but with separate fields (or maybe even just a single map) to supply the additional details e.g. x2, x4, x16. I also find that mixing the kind of antialiasing with the parameters in a single enum is smellier than keeping those things distinct, but I don't like methods with a lot of parameters either. You could supply simple static fields or methods to get a class configured for the common cases so it could be used as easily as an enum.
Scott On 2013-07-15, at 4:45 PM, Richard Bair <richard.b...@oracle.com> wrote: > >> I think there should be a simple way to request full scene anti-aliasing to >> improve 3D rendering. And also an optional more advanced way to specify >> which type… > > > I agree it should be nice and simple. It should also allow us the freedom to > make it better in the future. I think that adding a secondary property to > refine what is meant by the first property is not a good idea for the > following reason. As our API stands today it is not possible to later add any > API without cluttering things up. Specifically this is because we require you > to specify anti-aliasing at the time the scene is constructed. Ostensibly > this is because it is easier for our implementation. I think this is wrong > (and the fact that we expose a read only property for anti-aliasing is going > to come back to bite us if we ever attempt to make it a read-write property). > But suppose we decide that no, it really should be read-only for all time. In > that case, in 8.1 we decide to add more API that lets you specify various > details about the implementation, such as the algorithm to use. Now we have > to add yet another constructor with yet more parameters to be able to specify > this additional constraint. > > This is bad. > > What we can do is either of the following: > a) Make anti-aliasing a full read-write property and get it out of the > constructor > b) Change it to an enum (or class) so we can grow it over time > > Making it a read-write property (a) would at least allow us to add another > read-write property in the future (which is inevitable) without adding more > and more constructors. Once upon a time the root node of a Scene was > immutable and thus we had to have a non-empty scene constructor. That is no > longer the case and we should be striving to make Scene something that could > be completely configured dynamically (after all, we should be able to detach > and chuck the peer and create a new one on demand -- if we can't, that's a > failing in our implementation, not in the API). As such, it really is a bad > idea for us to be adding new read-only properties here. > > Separate from that discussion is the one around whether it should be a > boolean or an enum. I think a boolean is short-sighted because it is > inevitable that we are going to need to expose additional choices to > developers. So that means we will, at a minimum, need two properties instead > of one to configure the antialiasing strategy. It isn't the end of the world. > Using a full class to define the AA approach is also possible (if null, no > AA, otherwise it is an implementation of SceneAntiAliasing which could be > MSAA or whatever, package private constructor, fixed set of types, various > properties can grow on those different types over time, etc). Using a full > class probably provides the most flexibility but is more cumbersome. Using an > enum is just as easy programatically as a boolean, offers the opportunity to > grow it over time, but isn't as flexible as using a class. > > Once we ship this API we have to live with it forever, and I have a very > strong feeling that we are going to be modifying this in the future, because > as it stands it isn't that useful (only in the short term). > > I think making this a read-write property now is a must-do, because you can't > change it later (at least, you can't change the property method to return a > full property, it has to remain a read-only property forever for binary > compatibility reasons). > >> For the advanced variation, I think it might be premature. Considering there >> is an ugly workaround to specify number of samples for MSAA, with >> “prism.multisample” system property. > > We don't have to do anything prematurely. An enum with two options is > perfectly acceptable in the first go-around. The question is, how are we > going to handle growing this API in the future. Two properties in this case > is uglier than one. > > Richard
